[PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[hengyul]

From: Hengyu Liang <hengyul@cs.unc.edu>

Before commit 18e2bf0edf4d ("eventpoll: Add epoll ioctl for
epoll_params"), epoll files had no ioctl handler, so ioctl() on an epoll
file descriptor failed with ENOTTY. That commit introduced the
EPIOCSPARAMS and EPIOCGPARAMS commands, but ep_eventpoll_ioctl() returns
-EINVAL for any other command, so since v6.9 every other ioctl() on an
epoll file descriptor fails with EINVAL instead of ENOTTY.

Documentation/driver-api/ioctl.rst says that an ioctl handler must
return -ENOTTY or -ENOIOCTLCMD for an unknown command, and that
returning -EINVAL there is wrong. Returning -ENOIOCTLCMD was also the
intent of the original series, whose changelog since v3 [1] says "when
an unknown ioctl is received, -ENOIOCTLCMD is returned instead of
-EINVAL as the ioctl documentation requires", and ep_eventpoll_bp_ioctl()
does return -ENOIOCTLCMD for unknown commands. However,
ep_eventpoll_ioctl() only passes EPIOCSPARAMS and EPIOCGPARAMS to it and
handles all other commands in its own default case, which returns
-EINVAL, so that path is never reached.

This is visible to userspace. For example, isatty(), ttyname() and
tcgetattr() on an epoll file descriptor set errno to EINVAL, while they
set ENOTTY for any other file descriptor that does not refer to a
terminal, as they also did for epoll file descriptors before v6.9.

Return -ENOIOCTLCMD from the default case, which the VFS turns into
-ENOTTY, and update the epoll_busy_poll selftest, which expected EINVAL
for an unknown command.

[1] https://lore.kernel.org/r/20240125225704.12781-1-jdamato@fastly.com

Fixes: 18e2bf0edf4d ("eventpoll: Add epoll ioctl for epoll_params")
Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
 fs/eventpoll.c                                | 2 +-
 tools/testing/selftests/net/epoll_busy_poll.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index e0c4bf88a838..adf30b720b13 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1264,7 +1264,7 @@ static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd,
 		ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
 		break;
 	default:
-		ret = -EINVAL;
+		ret = -ENOIOCTLCMD;
 		break;
 	}
 
diff --git a/tools/testing/selftests/net/epoll_busy_poll.c b/tools/testing/selftests/net/epoll_busy_poll.c
index adf8dd0b5e0b..6b0b3213ffad 100644
--- a/tools/testing/selftests/net/epoll_busy_poll.c
+++ b/tools/testing/selftests/net/epoll_busy_poll.c
@@ -313,8 +313,8 @@ TEST_F(epoll_busy_poll, test_invalid_ioctl)
 	EXPECT_EQ(-1, ret)
 		TH_LOG("invalid ioctl should return error");
 
-	EXPECT_EQ(EINVAL, errno)
-		TH_LOG("invalid ioctl should set errno to EINVAL");
+	EXPECT_EQ(ENOTTY, errno)
+		TH_LOG("invalid ioctl should set errno to ENOTTY");
 }
 
 TEST_HARNESS_MAIN
-- 
2.53.0

Re: [PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[Joe Damato]

On Thu, Sep 24, 2026 at 02:57:47PM -0400, hengyul@cs.unc.edu wrote:
> From: Hengyu Liang <hengyul@cs.unc.edu>
> 
> Before commit 18e2bf0edf4d ("eventpoll: Add epoll ioctl for
> epoll_params"), epoll files had no ioctl handler, so ioctl() on an epoll
> file descriptor failed with ENOTTY. That commit introduced the
> EPIOCSPARAMS and EPIOCGPARAMS commands, but ep_eventpoll_ioctl() returns
> -EINVAL for any other command, so since v6.9 every other ioctl() on an
> epoll file descriptor fails with EINVAL instead of ENOTTY.
> 
> Documentation/driver-api/ioctl.rst says that an ioctl handler must
> return -ENOTTY or -ENOIOCTLCMD for an unknown command, and that
> returning -EINVAL there is wrong. Returning -ENOIOCTLCMD was also the
> intent of the original series, whose changelog since v3 [1] says "when
> an unknown ioctl is received, -ENOIOCTLCMD is returned instead of
> -EINVAL as the ioctl documentation requires", and ep_eventpoll_bp_ioctl()
> does return -ENOIOCTLCMD for unknown commands. However,
> ep_eventpoll_ioctl() only passes EPIOCSPARAMS and EPIOCGPARAMS to it and
> handles all other commands in its own default case, which returns
> -EINVAL, so that path is never reached.
> 
> This is visible to userspace. For example, isatty(), ttyname() and
> tcgetattr() on an epoll file descriptor set errno to EINVAL, while they
> set ENOTTY for any other file descriptor that does not refer to a
> terminal, as they also did for epoll file descriptors before v6.9.
> 
> Return -ENOIOCTLCMD from the default case, which the VFS turns into
> -ENOTTY, and update the epoll_busy_poll selftest, which expected EINVAL
> for an unknown command.
> 
> [1] https://lore.kernel.org/r/20240125225704.12781-1-jdamato@fastly.com
> 
> Fixes: 18e2bf0edf4d ("eventpoll: Add epoll ioctl for epoll_params")
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
>  fs/eventpoll.c                                | 2 +-
>  tools/testing/selftests/net/epoll_busy_poll.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index e0c4bf88a838..adf30b720b13 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1264,7 +1264,7 @@ static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd,
>  		ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
>  		break;
>  	default:
> -		ret = -EINVAL;
> +		ret = -ENOIOCTLCMD;
>  		break;
>  	}

I think based on the documentation this is probably right, but I am now
wondering why both ep_eventpoll_ioctl and ep_eventpoll_bp_ioctl need to
exist.

Maybe when I first implemented this I thought it made sense to factor
out the busy poll ioctls into their own function, but in retrospect maybe it's
cleaner to just collapse the ioctl function into a single one instead of
having two layers?

In other words, maybe:
  - delete ep_eventpoll_ioctl
  - add the is_file_epoll check to ep_eventpoll_bp_ioctl
  - rename ep_eventpoll_bp_ioctl to ep_eventpoll_ioctl
  - fix the test (as you did in this version of the patch)

Would result in a cleaner fewer helpers / cleaner code ?

Re: [PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[Hengyu Liang]

On Fri, Sep 25, 2026 at 5:59 AM Joe Damato <joe@dama.to> wrote:
>
> On Thu, Sep 24, 2026 at 02:57:47PM -0400, hengyul@cs.unc.edu wrote:

[...]

> >       default:
> > -             ret = -EINVAL;
> > +             ret = -ENOIOCTLCMD;
> >               break;
> >       }
>
> I think based on the documentation this is probably right, but I am now
> wondering why both ep_eventpoll_ioctl and ep_eventpoll_bp_ioctl need to
> exist.
>
> Maybe when I first implemented this I thought it made sense to factor
> out the busy poll ioctls into their own function, but in retrospect maybe
> it's cleaner to just collapse the ioctl function into a single one
> instead of having two layers?
>
> In other words, maybe:
>   - delete ep_eventpoll_ioctl
>   - add the is_file_epoll check to ep_eventpoll_bp_ioctl
>   - rename ep_eventpoll_bp_ioctl to ep_eventpoll_ioctl
>   - fix the test (as you did in this version of the patch)
>
> Would result in a cleaner fewer helpers / cleaner code ?

Thanks for taking a look. Agreed, a single handler would be cleaner.
Two things I noticed while looking into it:

1. With CONFIG_NET_RX_BUSY_POLL=n, ep_eventpoll_bp_ioctl() is the stub
   that returns -EOPNOTSUPP for every command. If it became the
   .unlocked_ioctl handler as is, every ioctl on an epoll fd would fail
   with EOPNOTSUPP on those kernels, which is the same problem in a
   different config. So the stub would need to keep a small switch:

    static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd,
                                   unsigned long arg)
    {
        switch (cmd) {
        case EPIOCSPARAMS:
        case EPIOCGPARAMS:
            return -EOPNOTSUPP;
        default:
            return -ENOIOCTLCMD;
        }
    }

2. The is_file_epoll() check cannot fail there: the handler is only
   reachable through eventpoll_fops, so file->f_op is always
   &eventpoll_fops. Unless you would like to keep it as a defensive
   check, I'd drop it rather than move it.

Since this changes the errno userspace sees and 18e2bf0edf4d is in
6.12 and 6.18, I'd like to keep the fix itself minimal so it backports
cleanly. How about a two-patch v2:

  1/2 this patch unchanged (Fixes: 18e2bf0edf4d)
  2/2 fold ep_eventpoll_bp_ioctl() into ep_eventpoll_ioctl() as you
      suggested, no functional change

If you'd prefer a single patch, I'm happy to do that instead.

Re: [PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[Joe Damato]

On Wed, Sep 30, 2026 at 05:50:58AM -0400, Hengyu Liang wrote:
> On Fri, Sep 25, 2026 at 5:59 AM Joe Damato <joe@dama.to> wrote:
> >
> > On Thu, Sep 24, 2026 at 02:57:47PM -0400, hengyul@cs.unc.edu wrote:
> 
> [...]
> 
> > >       default:
> > > -             ret = -EINVAL;
> > > +             ret = -ENOIOCTLCMD;
> > >               break;
> > >       }
> >
> > I think based on the documentation this is probably right, but I am now
> > wondering why both ep_eventpoll_ioctl and ep_eventpoll_bp_ioctl need to
> > exist.
> >
> > Maybe when I first implemented this I thought it made sense to factor
> > out the busy poll ioctls into their own function, but in retrospect maybe
> > it's cleaner to just collapse the ioctl function into a single one
> > instead of having two layers?
> >
> > In other words, maybe:
> >   - delete ep_eventpoll_ioctl
> >   - add the is_file_epoll check to ep_eventpoll_bp_ioctl
> >   - rename ep_eventpoll_bp_ioctl to ep_eventpoll_ioctl
> >   - fix the test (as you did in this version of the patch)
> >
> > Would result in a cleaner fewer helpers / cleaner code ?
> 
> Thanks for taking a look. Agreed, a single handler would be cleaner.
> Two things I noticed while looking into it:
> 
> 1. With CONFIG_NET_RX_BUSY_POLL=n, ep_eventpoll_bp_ioctl() is the stub
>    that returns -EOPNOTSUPP for every command. If it became the
>    .unlocked_ioctl handler as is, every ioctl on an epoll fd would fail
>    with EOPNOTSUPP on those kernels, which is the same problem in a
>    different config. So the stub would need to keep a small switch:
> 
>     static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd,
>                                    unsigned long arg)
>     {
>         switch (cmd) {
>         case EPIOCSPARAMS:
>         case EPIOCGPARAMS:
>             return -EOPNOTSUPP;
>         default:
>             return -ENOIOCTLCMD;
>         }
>     }

Yea, I see. I read it more closely this time. I suspect when I wrote this
originally, I had factored the code this way that way the stub could take care
of the CONFIG_NET_RX_BUSY_POLL=n case. So, now I'm not sure it make sense to
fold them into a single handler becauase we'd end up duplicating the switch
logic in two places for each CONFIG_NET_RX_BUSY_POLL setting.

So, in retrospect, I think probably the original patch you proposed might be
the best option. Sorry if I misread it the first time. 

Re: [PATCH] eventpoll: return -ENOIOCTLCMD for unknown ioctl commands

[Joe Damato]

On Thu, Sep 24, 2026 at 02:57:47PM -0400, hengyul@cs.unc.edu wrote:
> From: Hengyu Liang <hengyul@cs.unc.edu>
> 
> Before commit 18e2bf0edf4d ("eventpoll: Add epoll ioctl for
> epoll_params"), epoll files had no ioctl handler, so ioctl() on an epoll
> file descriptor failed with ENOTTY. That commit introduced the
> EPIOCSPARAMS and EPIOCGPARAMS commands, but ep_eventpoll_ioctl() returns
> -EINVAL for any other command, so since v6.9 every other ioctl() on an
> epoll file descriptor fails with EINVAL instead of ENOTTY.
> 
> Documentation/driver-api/ioctl.rst says that an ioctl handler must
> return -ENOTTY or -ENOIOCTLCMD for an unknown command, and that
> returning -EINVAL there is wrong. Returning -ENOIOCTLCMD was also the
> intent of the original series, whose changelog since v3 [1] says "when
> an unknown ioctl is received, -ENOIOCTLCMD is returned instead of
> -EINVAL as the ioctl documentation requires", and ep_eventpoll_bp_ioctl()
> does return -ENOIOCTLCMD for unknown commands. However,
> ep_eventpoll_ioctl() only passes EPIOCSPARAMS and EPIOCGPARAMS to it and
> handles all other commands in its own default case, which returns
> -EINVAL, so that path is never reached.
> 
> This is visible to userspace. For example, isatty(), ttyname() and
> tcgetattr() on an epoll file descriptor set errno to EINVAL, while they
> set ENOTTY for any other file descriptor that does not refer to a
> terminal, as they also did for epoll file descriptors before v6.9.
> 
> Return -ENOIOCTLCMD from the default case, which the VFS turns into
> -ENOTTY, and update the epoll_busy_poll selftest, which expected EINVAL
> for an unknown command.
> 
> [1] https://lore.kernel.org/r/20240125225704.12781-1-jdamato@fastly.com
> 
> Fixes: 18e2bf0edf4d ("eventpoll: Add epoll ioctl for epoll_params")
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
>  fs/eventpoll.c                                | 2 +-
>  tools/testing/selftests/net/epoll_busy_poll.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Joe Damato <joe@dama.to>