Hi Christian.
> > @@ -3736,6 +3853,18 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> > ctx->noswap = true;
> > ctx->seen |= SHMEM_SEEN_NOSWAP;
> > break;
> > + case Opt_quota:
> > + ctx->seen |= SHMEM_SEEN_QUOTA;
> > + ctx->quota_types |= (QTYPE_MASK_USR | QTYPE_MASK_GRP);
> > + break;
> > + case Opt_usrquota:
> > + ctx->seen |= SHMEM_SEEN_QUOTA;
> > + ctx->quota_types |= QTYPE_MASK_USR;
> > + break;
> > + case Opt_grpquota:
> > + ctx->seen |= SHMEM_SEEN_QUOTA;
> > + ctx->quota_types |= QTYPE_MASK_GRP;
> > + break;
> > }
> > return 0;
>
> I mentioned this in an earlier review; following the sequence:
Ok, my apologies, I should have lost it in the noise.
>
> if (ctx->seen & SHMEM_SEEN_QUOTA)
> -> shmem_enable_quotas()
> -> dquot_load_quota_sb()
>
> to then figure out that in dquot_load_quota_sb() we fail if
> sb->s_user_ns != &init_user_ns is too subtle for a filesystem that's
> mountable by unprivileged users. Every few months someone will end up
> stumbling upon this code and wonder where it's blocked. There isn't even
> a comment in the code.
>
> Aside from that it's also really unfriendly to users because they may go
> through setting up a tmpfs instances in the following way:
>
> fd_fs = fsopen("tmpfs");
>
> User now enables quota:
>
> fsconfig(fd_fs, ..., "quota", ...) = 0
>
> and goes on to set a bunch of other options:
>
> fsconfig(fd_fs, ..., "inode64", ...) = 0
> fsconfig(fd_fs, ..., "nr_inodes", ...) = 0
> fsconfig(fd_fs, ..., "nr_blocks", ...) = 0
> fsconfig(fd_fs, ..., "huge", ...) = 0
> fsconfig(fd_fs, ..., "mode", ...) = 0
> fsconfig(fd_fs, ..., "gid", ...) = 0
>
> everything seems dandy and they create the superblock:
>
> fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...) = -EINVAL
>
> which fails.
>
> The user has not just performed 9 useless system calls they also have
> zero clue what mount option caused the failure.
>
> What this code really really should do is fail at:
>
> fsconfig(fd_fs, ..., "quota", ...) = -EINVAL
>
> and log an error that the user can retrieve from the fs context. IOW,
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 083ce6b478e7..baca8bf44569 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3863,14 +3863,20 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> ctx->seen |= SHMEM_SEEN_NOSWAP;
> break;
> case Opt_quota:
> + if (fc->user_ns != &init_user_ns)
> + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported");
> ctx->seen |= SHMEM_SEEN_QUOTA;
> ctx->quota_types |= (QTYPE_MASK_USR | QTYPE_MASK_GRP);
> break;
> case Opt_usrquota:
> + if (fc->user_ns != &init_user_ns)
> + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported");
> ctx->seen |= SHMEM_SEEN_QUOTA;
> ctx->quota_types |= QTYPE_MASK_USR;
> break;
> case Opt_grpquota:
> + if (fc->user_ns != &init_user_ns)
> + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported");
> ctx->seen |= SHMEM_SEEN_QUOTA;
> ctx->quota_types |= QTYPE_MASK_GRP;
> break;
>
> This exactly what we already to for the "noswap" option btw.
>
> Could you fold these changes into the patch and resend, please?
> I synced with Andrew earlier and I'll be taking this series.
Thanks! I will sure do it, I'll update the patch, build, test and send it again
in a few minutes.
>
> ---
>
> And btw, the *_SEEN_* logic for mount options is broken - but that's not
> specific to your patch. Imagine:
>
> fd_fs = fsopen("tmpfs");
> fsconfig(fd_fs, ..., "nr_inodes", 0, "1000") = 0
>
> Now ctx->inodes == 1000 and ctx->seen |= SHMEM_SEEN_INODES.
>
> Now the user does:
>
> fsconfig(fd_fs, ..., "nr_inodes", 0, "-1234") = -EINVAL
>
> This fails, but:
>
> ctx->inodes = memparse(param->string, &rest);
> if (*rest)
> goto bad_value;
>
> will set ctx->inodes to whatever memparse returns but leaves
> SHMEM_SEEN_INODES raised in ctx->seen. Now superblock creation may
> succeed with a garbage inode limit. This should affect other mount
> options as well.
Interesting. Thanks for the heads up. I'll look in more details into it when I
start working for namespace support for quotas (as we spoke previously).
Cheers.
--
Carlos
