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?
>
> * 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
probably best to do both at the same time, or maybe
have a single callback pointer with an in/out flag.
Arnd
