Re: [PATCH 4/7] namei: clean up do_mknodat retry logic

[Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>]

On Mon, Jul 12, 2021 at 5:37 AM Dmitry Kadashev <dkadashev@xxxxxxxxx> wrote:
>
> Moving the main logic to a helper function makes the whole thing much
> easier to follow.

This patch works, but the conversion is not one-to-one.

> -static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> -               unsigned int dev)
> +static int mknodat_helper(int dfd, struct filename *name, umode_t mode,
> +                         unsigned int dev, unsigned int lookup_flags)
>  {
>         struct user_namespace *mnt_userns;
>         struct dentry *dentry;
>         struct path path;
>         int error;
> -       unsigned int lookup_flags = 0;
>
>         error = may_mknod(mode);
>         if (error)
> -               goto out1;
> -retry:

Note how "may_mknod()" was _outside_ the retry before, and now it's inside:

> +static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> +               unsigned int dev)
> +{
> +       int error;
> +
> +       error = mknodat_helper(dfd, name, mode, dev, 0);
> +       if (retry_estale(error, 0))
> +               error = mknodat_helper(dfd, name, mode, dev, LOOKUP_REVAL);
> +
>         putname(name);
>         return error;

which happens to not be fatal - doing the may_mknod() twice will not
break anything - but it doesn't match what it used to do.

A few options options:

 (a) a separate patch to move the "may_mknod()" to the two callers first

 (b) a separate patch to move the "may_mknod()" first into the repeat
loop, with the comment being that it's ok.

 (c) keep the logic the same, with the code something like

  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
                unsigned int dev)
  {
        int error;

        error = may_mknod(mode);
        if (!error) {
                error = mknodat_helper(dfd, name, mode, dev, 0);
                if (retry_estale(error, 0))
                        error = mknodat_helper(dfd, name, mode, dev,
LOOKUP_REVAL);
        }

        putname(name);
        return error;
  }

or

 (d) keep the patch as-is, but with an added commit message note about
how it's not one-to-one and why it's ok.

Hmm?

So this patch could be fine, but it really wants to note how it
changes the logic and why that's fine. Or, the patch should be
modified.

                Linus