On Sat, Feb 10, 2024 at 12:49:23PM +0100, Arnd Bergmann wrote:
> On Sat, Feb 10, 2024, at 12:06, Günther Noack wrote:
> > On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
> >
> > The IOCTL command in question is FIONREAD: fs/ioctl.c implements
> > FIONREAD directly for S_ISREG files, but it does call the FIONREAD
> > implementation in the VFS layer for other file types.
> >
> > The question we are asking ourselves is:
> >
> > * Can we let processes safely use FIONREAD for all files which get
> > opened for the purpose of reading, or do we run the risk of
> > accidentally exposing surprising IOCTL implementations which have
> > completely different purposes?
> >
> > Is it safe to assume that the VFS layer FIONREAD implementations are
> > actually implementing FIONREAD semantics?
Yes, otherwise this should considered a bug.
> >
> > * I know there have been accidental collisions of IOCTL command
> > numbers in the past -- Hypothetically, if this were to happen in one
> > of the VFS implementations of FIONREAD, would that be considered a
> > bug that would need to get fixed in that implementation?
>
> Clearly it's impossible to be sure no driver has a conflict
> on this particular ioctl, but the risk for one intentionally
> overriding it should be fairly low.
>
> There are a couple of possible issues I can think of:
>
> - the numeric value of FIONREAD is different depending
> on the architecture, with at least four different numbers
> aliasing to it. This is probably harmless but makes it
> harder to look for accidental conflicts.
>
> - Aside from FIONREAD, it is sometimes called SIOCINQ
> (for sockets) or TIOCINQ (for tty). These still go
> through the same VFS entry point and as far as I can
> tell always have the same semantics (writing 4 bytes
> of data with the count of the remaining bytes in the
> fd).
>
> - There are probably a couple of drivers that do something
> in their ioctl handler without actually looking at
> the command number.
>
> If you want to be really sure you get this right, you
> could add a new callback to struct file_operations
> that handles this for all drivers, something like
>
> static int ioctl_fionread(struct file *filp, int __user *arg)
> {
> int n;
>
> if (S_ISREG(inode->i_mode))
> return put_user(i_size_read(inode) - filp->f_pos, arg);
>
> if (!file->f_op->fionread)
> return -ENOIOCTLCMD;
>
> n = file->f_op->fionread(filp);
>
> if (n < 0)
> return n;
>
> return put_user(n, arg);
> }
>
> With this, you can go through any driver implementing
> FIONREAD/SIOCINQ/TIOCINQ and move the code from .ioctl
> into .fionread. This probably results in cleaner code
> overall, especially in drivers that have no other ioctl
> commands besides this one.
>
> Since sockets and ttys tend to have both SIOCINQ/TIOCINQ
> and SIOCOUTQ/TIOCOUTQ (unlike regular files), it's
I'm not excited about adding a bunch of methods to struct
file_operations for this stuff.
