[PATCH net-next] tap: report IFF_DETACH_QUEUE in TUNGETIFF

[PATCH net-next] tap: report IFF_DETACH_QUEUE in TUNGETIFF

[Umang Pokhriyal]

tun sets IFF_DETACH_QUEUE in the flags returned by TUNGETIFF when the
queue is detached, since commit 3d407a80b62f ("tun: Report whether the
queue is attached or not"). tap_ioctl() returns only q->flags, so a
detached macvtap or ipvtap queue is reported as attached.

Cloud Hypervisor ran into this when checking queue state on macvtap.

Report the flag from q->enabled, as tun does.

Assisted-by: LLM
Signed-off-by: Umang Pokhriyal <umangpokhriyall@gmail.com>
---
 drivers/net/tap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index ff67d99deb39e..cb8ffb0c4a315 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -950,6 +950,8 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
 
 		ret = 0;
 		u = q->flags;
+		if (!q->enabled)
+			u |= IFF_DETACH_QUEUE;
 		if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
 		    put_user(u, &ifr->ifr_flags))
 			ret = -EFAULT;

base-commit: c66d93e68728cfb5f40b40d0f24129d7768faf43
-- 
2.53.0

Re: [PATCH net-next] tap: report IFF_DETACH_QUEUE in TUNGETIFF

[Willem de Bruijn]

Umang Pokhriyal wrote:
> tun sets IFF_DETACH_QUEUE in the flags returned by TUNGETIFF when the
> queue is detached, since commit 3d407a80b62f ("tun: Report whether the
> queue is attached or not"). tap_ioctl() returns only q->flags, so a
> detached macvtap or ipvtap queue is reported as attached.
> 
> Cloud Hypervisor ran into this when checking queue state on macvtap.
> 
> Report the flag from q->enabled, as tun does.
> 
> Assisted-by: LLM
> Signed-off-by: Umang Pokhriyal <umangpokhriyall@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next] tap: report IFF_DETACH_QUEUE in TUNGETIFF

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In tap_ioctl() (drivers/net/tap.c), TUNGETIFF now sets
  IFF_DETACH_QUEUE (0x0400) in the returned flags when q->enabled is…
- [Low] The commit message describes a user-visible bug: tap_ioctl()
  TUNGETIFF reports a detached macvtap/ipvtap queue as attached, and…

--

tap: report IFF_DETACH_QUEUE in TUNGETIFF

With this commit, tap_ioctl() sets IFF_DETACH_QUEUE in the flags that
TUNGETIFF returns when the macvtap/ipvtap queue is disabled. tun has
reported the flag this way since commit 3d407a80b62f.

> tap: report IFF_DETACH_QUEUE in TUNGETIFF
>
> tun sets IFF_DETACH_QUEUE in the flags returned by TUNGETIFF when the
> queue is detached, since commit 3d407a80b62f ("tun: Report whether the
> queue is attached or not"). tap_ioctl() returns only q->flags, so a
> detached macvtap or ipvtap queue is reported as attached.
>
> Cloud Hypervisor ran into this when checking queue state on macvtap.

[Severity: Low]
The message describes a bug that users can see, but there is no Fixes:
tag.

Commit 815f236d6227 ("macvtap: add TUNSETQUEUE ioctl") added queue detach
for macvtap. Since then, TUNGETIFF in tap_ioctl() has returned only
q->flags. Commit 3d407a80b62f later added IFF_DETACH_QUEUE reporting, but
only for tun.

If this is meant for net and stable, should it have a tag like this?

Fixes: 815f236d6227 ("macvtap: add TUNSETQUEUE ioctl")

If it is meant as feature parity for net-next, could the commit message
say so instead of calling it a bug?

> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index ff67d99deb39e..cb8ffb0c4a315 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -950,6 +950,8 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
>  
>  		ret = 0;
>  		u = q->flags;
> +		if (!q->enabled)
> +			u |= IFF_DETACH_QUEUE;

[Severity: Medium]
Can this break a read-modify-write of TUNGETIFF and TUNSETIFF on a
detached queue?

The TUNSETIFF case in the same function was not changed. It still rejects
any bit outside TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE) other than
IFF_NO_PI | IFF_TAP:

drivers/net/tap.c:tap_ioctl() {
	case TUNSETIFF:
		/* ignore the name, just look at flags */
		if (get_user(u, &ifr->ifr_flags))
			return -EFAULT;

		ret = 0;
		if ((u & ~TAP_IFFEATURES) != (IFF_NO_PI | IFF_TAP))
			ret = -EINVAL;
		else
			q->flags = (q->flags & ~TAP_IFFEATURES) | u;
}

Unlike tun, tap accepts TUNSETIFF again on an fd that is already bound.
Userspace can use this to change IFF_VNET_HDR or IFF_MULTI_QUEUE at
runtime.

With this patch, the following sequence now fails:

open /dev/tapN
  tap_open()                        q->enabled = true
TUNSETQUEUE(IFF_DETACH_QUEUE)
  tap_ioctl_set_queue()
    tap_disable_queue()             q->enabled = false
TUNGETIFF                           returns q->flags | IFF_DETACH_QUEUE
TUNSETIFF with that value           returns -EINVAL

Before this patch, TUNGETIFF returned exactly q->flags, truncated to
unsigned short. In its low 16 bits, q->flags only holds IFF_TAP,
IFF_NO_PI, IFF_VNET_HDR and IFF_MULTI_QUEUE. TUN_VNET_LE and TUN_VNET_BE
sit above bit 15. So writing the value back always passed the check above.

The commit message says:

    Report the flag from q->enabled, as tun does.

In __tun_chr_ioctl(), however, TUNSETIFF returns -EEXIST if the fd
already has a device attached:

drivers/net/tun.c:__tun_chr_ioctl() {
	tun = tun_get(tfile);
	if (cmd == TUNSETIFF) {
		ret = -EEXIST;
		if (tun)
			goto unlock;
}

So tun never had to handle its TUNGETIFF output being written back.

Should TUNSETIFF in tap_ioctl() mask off IFF_DETACH_QUEUE and
IFF_ATTACH_QUEUE? If not, should the commit message at least mention
this change in what userspace sees?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929142238.41742-1-umangpokhriyall%40gmail.com