diff mbox series

Alloc cap limit for 9p xattrs (Was: WARNING in __alloc_frozen_pages_noprof)

Message ID Z1n-Ue19Pa_AWVu0@codewreck.org (mailing list archive)
State New
Headers show
Series Alloc cap limit for 9p xattrs (Was: WARNING in __alloc_frozen_pages_noprof) | expand

Commit Message

Dominique Martinet Dec. 11, 2024, 9:04 p.m. UTC 
Leo Stone wrote on Wed, Dec 11, 2024 at 12:02:40PM -0800:
> syzbot creates a pipe and writes some data to it. It then creates a v9fs
> mount using the pipe as transport. The data in the pipe specifies an ACL
> of size 9 TB (9895604649984 bytes) for the root inode, causing kmalloc
> to fail.

grmbl.

Sorry about that, there's been some paches ages ago to either cap xattrs
allocations to XATTR_SIZE_MAX, KMALLOC_MAX_SIZE, look into
vfs_getxattr_alloc or just flag the alloc __GFP_NOWARN:
https://lore.kernel.org/all/20240304-xattr_maxsize-v1-1-322357ec6bdf@codewreck.org/T/#u

and it was left forgotten because no decision was taken on something I
don't have time to think about

I've re-added everyone involved in Ccs, let's pick one and be done with
it.

Christian Schoenebeck's suggestion was something like this -- I guess
that's good enough for now and won't break anything (e.g. ACLs bigger
than XATTR_SIZE_MAX), so shall we go with that instead?

I don't care but let's get something in this cycle, the first patch is
almost one year old and this is ridiculous...


--
Dominique,
sleepy

Comments

Linus Torvalds Dec. 11, 2024, 9:32 p.m. UTC | #1 
On Wed, 11 Dec 2024 at 13:04, <asmadeus@codewreck.org> wrote:
>
> Christian Schoenebeck's suggestion was something like this -- I guess
> that's good enough for now and won't break anything (e.g. ACLs bigger
> than XATTR_SIZE_MAX), so shall we go with that instead?

Please use XATTR_SIZE_MAX. The KMALLOC_MAX_SIZE limit seems to make no
sense in this context.

Afaik the VFS layer doesn't allow getting an xattr bigger than
XATTR_SIZE_MAX anyway, and would return E2BIG for them later
regardless, so returning anything bigger wouldn't work anyway, even if
p9 tried to return such a thing up to some bigger limit.

No?

           Linus
Al Viro Dec. 11, 2024, 10:55 p.m. UTC | #2 
On Wed, Dec 11, 2024 at 01:32:26PM -0800, Linus Torvalds wrote:
> On Wed, 11 Dec 2024 at 13:04, <asmadeus@codewreck.org> wrote:
> >
> > Christian Schoenebeck's suggestion was something like this -- I guess
> > that's good enough for now and won't break anything (e.g. ACLs bigger
> > than XATTR_SIZE_MAX), so shall we go with that instead?
> 
> Please use XATTR_SIZE_MAX. The KMALLOC_MAX_SIZE limit seems to make no
> sense in this context.
> 
> Afaik the VFS layer doesn't allow getting an xattr bigger than
> XATTR_SIZE_MAX anyway, and would return E2BIG for them later
> regardless, so returning anything bigger wouldn't work anyway, even if
> p9 tried to return such a thing up to some bigger limit.

E2BIG on attempt to set, quiet cap to XATTR_SIZE_MAX on attempt to get
(i.e. never asking more than that from fs) and if filesystem complains
about XATTR_SIZE_MAX not being enough, E2BIG it is (instead of ERANGE
normally expected on "your buffer is too small for that").
diff mbox series

Patch

diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index 8604e3377ee7..97f60b73bf16 100644
--- a/fs/9p/xattr.c
+++ b/fs/9p/xattr.c
@@ -37,8 +37,8 @@  ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name,
 	if (attr_size > buffer_size) {
 		if (buffer_size)
 			retval = -ERANGE;
-		else if (attr_size > SSIZE_MAX)
-			retval = -EOVERFLOW;
+		else if (attr_size > KMALLOC_MAX_SIZE)
+			retval = -E2BIG;
 		else /* request to get the attr_size */
 			retval = attr_size;
 	} else {