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

[Joe Damato]

On Thu, Sep 24, 2026 at 02:57:47PM -0400, hengyul@xxxxxxxxxx wrote:
> From: Hengyu Liang <hengyul@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx
> 
> Fixes: 18e2bf0edf4d ("eventpoll: Add epoll ioctl for epoll_params")
> Signed-off-by: Hengyu Liang <hengyul@xxxxxxxxxx>
> ---
>  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 ?