Re: [RFC] struct fd situation

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, May 31, 2024 at 04:18:02AM +0100, Al Viro wrote:

> Alternative would be to turn struct fd into a struct-wrapped unsigned long
> and either use a flag for emptiness checks (we can steal more from the
> pointer) or just compare with zero for empitness check.
> 
> The former might allow to represent specific errors, which would give a neat
> solution for ovl_real_fd() calling conventions - instead of "return an error,
> fill user-supplied struct fd" it could just return an error-representing
> struct fd and be done with that.
> 
> Unfortunately, gcc optimizer really stinks when it comes to bitops -
[snip]

> That example is very close to what we'd need if fd_empty() turned
> into checking a flag.  So unless somebody has a clever scheme that
> would avoid that kind of fun, this is probably no-go.
> 
> If we give up on representing errors, we can use 0 for empty.

Hmm...  FWIW, we could do something like this:

fd: 0 for empty, (unsigned long)p | flags, with p being an address of struct file
fd_err: a non-zero value possible for fd or (unsigned long)-E... for an error.

The range of valid values for fd does not overlap with
	(unsigned long)-MAX_ERRNO..(unsigned long)-1
since that would require IS_ERR(p) to be true and if we ever had that for
an address of some object, we would have worse problems.

That would be something along the lines of

#define fd_file(f) (struct file *)((f).word & ~3)

static inline bool fd_empty(struct fd f)
{
	return unlikely(!f.word);
}

static inline void fdput(struct fd f)
{
	if (f.word & FDPUT_FPUT)
		fput(fd_file(f));
}

static inline bool __must_check FD_IS_ERR(struct fd_err f)
{
	return IS_ERR_VALUE(f.word);
}

static inline void fdput_err(struct fd_err f)
{
	if (!FD_IS_ERR(f))
		fdput((struct fd){f.word});
}

static inline long __must_check FD_ERR(struct fd_err f)
{
	return (long)fd.word;
}

That's basically the difference between "pointer to object or NULL" and
"pointer to object or ERR_PTR()", only with distinguishable types for
those...

Interesting...  So we get something like

static inline struct fd_err file_fd_err(struct file *f, bool is_cloned)
{
	if (IS_ERR(f))
		return (struct fd_err){PTR_ERR(f)};
	else
		return (struct fd_err){(unsigned long)f | is_cloned};
}

(or, perhaps, file_fd_cloned and file_fd_borrowed, to get rid of
'is_cloned' argument in public API; need to play around a bit and
see what works better - the interesting part is what the constructor
for struct fd would look like)

with 

static struct fd_err ovl_real_fdget_meta(const struct file *file, bool allow_meta)
{
        struct dentry *dentry = file_dentry(file);
        struct path realpath;
        int err;

        if (allow_meta) {
                ovl_path_real(dentry, &realpath);
        } else {
                /* lazy lookup and verify of lowerdata */
                err = ovl_verify_lowerdata(dentry);
                if (err)
                        return file_fd_err(ERR_PTR(err), false);

                ovl_path_realdata(dentry, &realpath);
        }
        if (!realpath.dentry)
                return file_fd_err(ERR_PTR(-EIO), false);

        /* Has it been copied up since we'd opened it? */
        if (unlikely(file_inode(real->file) != d_inode(realpath.dentry)))
		return file_fd_err(ovl_open_realfile(file, &realpath), true);

        /* Did the flags change since open? */
        if (unlikely((file->f_flags ^ real->file->f_flags) & ~OVL_OPEN_FLAGS)) {
		err = ovl_change_flags(real->file, file->f_flags);
		if (err)
			return file_fd_err(ERR_PTR(err), false);
	}

        return file_fd_err(file->private_data, false);
}

static struct fd_err ovl_real_fdget(const struct file *file)
{
        if (d_is_dir(file_dentry(file)))
		return file_fd_err(ovl_dir_real_file(file, false), false);

        return ovl_real_fdget_meta(file, false);
}

with callers looking like

        real = ovl_real_fdget(file);
        if (FD_IS_ERR(real))
                return FD_ERR(real);
	do stuff, using fd_file(real)
	fdput_err(real);

with possibility of __cleanup() use, since fdput_err() on
early failure exit would be a no-op.