Re: introduce struct fderr, convert overlayfs uses to that

Re: introduce struct fderr, convert overlayfs uses to that

[Amir Goldstein]

On Fri, Oct 4, 2024 at 1:47 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Similar to struct fd; unlike struct fd, it can represent
> error values.
>
> Accessors:
>
> * fd_empty(f):  true if f represents an error
> * fd_file(f):   just as for struct fd it yields a pointer to
>                 struct file if fd_empty(f) is false.  If
>                 fd_empty(f) is true, fd_file(f) is guaranteed
>                 _not_ to be an address of any object (IS_ERR()
>                 will be true in that case)
> * fd_err(f):    if f represents an error, returns that error,
>                 otherwise the return value is junk.
>
> Constructors:
>
> * ERR_FDERR(-E...):     an instance encoding given error [ERR_FDERR, perhaps?]
> * BORROWED_FDERR(file): if file points to a struct file instance,
>                         return a struct fderr representing that file
>                         reference with no flags set.
>                         if file is an ERR_PTR(-E...), return a struct
>                         fderr representing that error.
>                         file MUST NOT be NULL.
> * CLONED_FDERR(file):   similar, but in case when file points to
>                         a struct file instance, set FDPUT_FPUT in flags.
>
> Same destructor as for struct fd; I'm not entirely convinced that
> playing with _Generic is a good idea here, but for now let's go
> that way...
>
> See fs/overlayfs/file.c for example of use.

I had already posted an alternative code for overlayfs, but in case this
is going to be used anyway in overlayfs or in another code, see some
comments below...

>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/overlayfs/file.c  | 128 +++++++++++++++++++++----------------------
>  include/linux/file.h |  37 +++++++++++--
>  2 files changed, 95 insertions(+), 70 deletions(-)
>

[...]

> diff --git a/include/linux/file.h b/include/linux/file.h
> index f98de143245a..d85352523368 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -44,13 +44,26 @@ static inline void fput_light(struct file *file, int fput_needed)
>  struct fd {
>         unsigned long word;
>  };
> +
> +/* either a reference to struct file + flags
> + * (cloned vs. borrowed, pos locked), with
> + * flags stored in lower bits of value,
> + * or an error (represented by small negative value).
> + */
> +struct fderr {
> +       unsigned long word;
> +};
> +
>  #define FDPUT_FPUT       1
>  #define FDPUT_POS_UNLOCK 2
>
> +#define fd_empty(f)    _Generic((f), \
> +                               struct fd: unlikely(!(f).word), \
> +                               struct fderr: IS_ERR_VALUE((f).word))


I suggest adding a fd_is_err(f) helper to rhyme with IS_ERR().

>  #define fd_file(f) ((struct file *)((f).word & ~(FDPUT_FPUT|FDPUT_POS_UNLOCK)))
> -static inline bool fd_empty(struct fd f)
> +static inline long fd_err(struct fderr f)
>  {
> -       return unlikely(!f.word);
> +       return (long)f.word;
>  }
>
>  #define EMPTY_FD (struct fd){0}
> @@ -63,11 +76,25 @@ static inline struct fd CLONED_FD(struct file *f)
>         return (struct fd){(unsigned long)f | FDPUT_FPUT};
>  }
>
> -static inline void fdput(struct fd fd)
> +static inline struct fderr ERR_FDERR(long n)
> +{
> +       return (struct fderr){(unsigned long)n};
> +}
> +static inline struct fderr BORROWED_FDERR(struct file *f)
>  {
> -       if (fd.word & FDPUT_FPUT)
> -               fput(fd_file(fd));
> +       return (struct fderr){(unsigned long)f};
>  }
> +static inline struct fderr CLONED_FDERR(struct file *f)
> +{
> +       if (IS_ERR(f))
> +               return BORROWED_FDERR(f);
> +       return (struct fderr){(unsigned long)f | FDPUT_FPUT};
> +}
> +
> +#define fdput(f)       (void) (_Generic((f), \
> +                               struct fderr: IS_ERR_VALUE((f).word),   \

Should that be !IS_ERR_VALUE((f).word)?

> +                               struct fd: true) && \
> +                           ((f).word & FDPUT_FPUT) && (fput(fd_file(f)),0))
>

or better yet

#define fd_is_err(f) _Generic((f), \
                                struct fd: false, \
                                struct fderr: IS_ERR_VALUE((f).word))
#define fdput(f) (!fd_is_err(f) && ((f).word & FDPUT_FPUT) &&
(fput(fd_file(f)),0))

Thanks,
Amir.

Re: introduce struct fderr, convert overlayfs uses to that

[Al Viro]

On Fri, Oct 04, 2024 at 12:47:09PM +0200, Amir Goldstein wrote:

> I had already posted an alternative code for overlayfs, but in case this
> is going to be used anyway in overlayfs or in another code, see some
> comments below...

As far as I can see, the current #overlayfs-next kills the case for
struct fderr; we might eventually get a valid use for it, but for the
time being I'm going to strip the overlayfs-related parts of that branch
(obviously), fix the braino you've spotted in fdput() and archive the
branch in case it's ever needed.

> > +#define fd_empty(f)    _Generic((f), \
> > +                               struct fd: unlikely(!(f).word), \
> > +                               struct fderr: IS_ERR_VALUE((f).word))
> 
> 
> I suggest adding a fd_is_err(f) helper to rhyme with IS_ERR().

Umm...  Dropping fd_empty() for that one, you mean?

> > +#define fdput(f)       (void) (_Generic((f), \
> > +                               struct fderr: IS_ERR_VALUE((f).word),   \
> 
> Should that be !IS_ERR_VALUE((f).word)?

It should, thanks for spotting that braino.
 
> or better yet
> 
> #define fd_is_err(f) _Generic((f), \
>                                 struct fd: false, \
>                                 struct fderr: IS_ERR_VALUE((f).word))

I think that's a bad idea; too likely to spill into struct fd users,
with "it's never false here" being a nasty surprise.