| Message ID | 20240911034608.43192-1-atlas.yu@canonical.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] dev_ioctl: fix the type of ifr_flags | expand |
On Wed, 11 Sep 2024 11:46:08 +0800 Atlas Yu <atlas.yu@canonical.com> wrote: > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h > index 797ba2c1562a..b612b6cd7446 100644 > --- a/include/uapi/linux/if.h > +++ b/include/uapi/linux/if.h > @@ -244,7 +244,7 @@ struct ifreq { > struct sockaddr ifru_broadaddr; > struct sockaddr ifru_netmask; > struct sockaddr ifru_hwaddr; > - short ifru_flags; > + unsigned int ifru_flags; > int ifru_ivalue; > int ifru_mtu; > struct ifmap ifru_map; NAK This breaks userspace ABI. There is no guarantee that older application correctly zeros the upper flag bits.
On Wed, Sep 11, 2024 at 11:56 AM Stephen Hemminger <stephen@networkplumber.org> wrote: > On Wed, 11 Sep 2024 11:46:08 +0800 > Atlas Yu <atlas.yu@canonical.com> wrote: > > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h > > index 797ba2c1562a..b612b6cd7446 100644 > > --- a/include/uapi/linux/if.h > > +++ b/include/uapi/linux/if.h > > @@ -244,7 +244,7 @@ struct ifreq { > > struct sockaddr ifru_broadaddr; > > struct sockaddr ifru_netmask; > > struct sockaddr ifru_hwaddr; > > - short ifru_flags; > > + unsigned int ifru_flags; > > int ifru_ivalue; > > int ifru_mtu; > > struct ifmap ifru_map; > > NAK > This breaks userspace ABI. There is no guarantee that > older application correctly zeros the upper flag bits. Thanks, any suggestions though? How about introducing another ioctl request for these extended bits.
On Wed, 11 Sep 2024 12:17:24 +0800 Atlas Yu <atlas.yu@canonical.com> wrote: > On Wed, Sep 11, 2024 at 11:56 AM > Stephen Hemminger <stephen@networkplumber.org> wrote: > > > On Wed, 11 Sep 2024 11:46:08 +0800 > > Atlas Yu <atlas.yu@canonical.com> wrote: > > > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h > > > index 797ba2c1562a..b612b6cd7446 100644 > > > --- a/include/uapi/linux/if.h > > > +++ b/include/uapi/linux/if.h > > > @@ -244,7 +244,7 @@ struct ifreq { > > > struct sockaddr ifru_broadaddr; > > > struct sockaddr ifru_netmask; > > > struct sockaddr ifru_hwaddr; > > > - short ifru_flags; > > > + unsigned int ifru_flags; > > > int ifru_ivalue; > > > int ifru_mtu; > > > struct ifmap ifru_map; > > > > NAK > > This breaks userspace ABI. There is no guarantee that > > older application correctly zeros the upper flag bits. > > Thanks, any suggestions though? How about introducing > another ioctl request for these extended bits. Doing anything with ioctl's is hard, there are so many layers to deal with. Mostly, networking has treated ioctl's as legacy API and switched to using netlink for any new features.
On Wed, Sep 11, 2024 at 7:00 AM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Wed, 11 Sep 2024 12:17:24 +0800 > Atlas Yu <atlas.yu@canonical.com> wrote: > > > On Wed, Sep 11, 2024 at 11:56 AM > > Stephen Hemminger <stephen@networkplumber.org> wrote: > > > > > On Wed, 11 Sep 2024 11:46:08 +0800 > > > Atlas Yu <atlas.yu@canonical.com> wrote: > > > > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h > > > > index 797ba2c1562a..b612b6cd7446 100644 > > > > --- a/include/uapi/linux/if.h > > > > +++ b/include/uapi/linux/if.h > > > > @@ -244,7 +244,7 @@ struct ifreq { > > > > struct sockaddr ifru_broadaddr; > > > > struct sockaddr ifru_netmask; > > > > struct sockaddr ifru_hwaddr; > > > > - short ifru_flags; > > > > + unsigned int ifru_flags; > > > > int ifru_ivalue; > > > > int ifru_mtu; > > > > struct ifmap ifru_map; > > > > > > NAK > > > This breaks userspace ABI. There is no guarantee that > > > older application correctly zeros the upper flag bits. > > > > Thanks, any suggestions though? How about introducing > > another ioctl request for these extended bits. > > Doing anything with ioctl's is hard, there are so many layers to deal with. > Mostly, networking has treated ioctl's as legacy API and switched to using > netlink for any new features. Note that rtnetlink gets the 32bit flags already, this has been the case for 20 years or more. include/uapi/linux/rtnetlink.h:566: unsigned ifi_flags; /* IFF_* flags */ net/core/rtnetlink.c : rtnl_fill_ifinfo(...) ifm->ifi_flags = dev_get_flags(dev); iproute2 also displays more than 16 bits in print_link_flags()
> On Wed, Sep 11, 2024 at 3:21 PM > Eric Dumazet <edumazet@google.com> wrote: > > Note that rtnetlink gets the 32bit flags already, this has been the > case for 20 years or more. > > include/uapi/linux/rtnetlink.h:566: unsigned ifi_flags; > /* IFF_* flags */ > > net/core/rtnetlink.c : rtnl_fill_ifinfo(...) > > ifm->ifi_flags = dev_get_flags(dev); > > iproute2 also displays more than 16 bits in print_link_flags() Thank you Eric, I am switching to the netlink API for my use case.
diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h index 797ba2c1562a..b612b6cd7446 100644 --- a/include/uapi/linux/if.h +++ b/include/uapi/linux/if.h @@ -244,7 +244,7 @@ struct ifreq { struct sockaddr ifru_broadaddr; struct sockaddr ifru_netmask; struct sockaddr ifru_hwaddr; - short ifru_flags; + unsigned int ifru_flags; int ifru_ivalue; int ifru_mtu; struct ifmap ifru_map; diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 8592c052c0f4..4b317561e503 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -145,7 +145,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm switch (cmd) { case SIOCGIFFLAGS: /* Get interface flags */ - ifr->ifr_flags = (short) dev_get_flags(dev); + ifr->ifr_flags = dev_get_flags(dev); return 0; case SIOCGIFMETRIC: /* Get the metric on the interface
SIOCxIFFAGS call dev_get_flags/dev_change_flags under the hood, which take a unsigned int flags instead of short. This mismatch makes it impossible to get/set IFF_LOWER_UP, IFF_DORMANT and IFF_ECHO that are beyond 16 bits. Signed-off-by: Atlas Yu <atlas.yu@canonical.com> --- include/uapi/linux/if.h | 2 +- net/core/dev_ioctl.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)