| Message ID | 20220920194825.31820-1-prohr@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 195624d9c26b64c6856863da30ec578a790feec4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] tun: support not enabling carrier in TUNSETIFF | expand |
On Tue, Sep 20, 2022 at 12:48 PM Patrick Rohr <prohr@google.com> wrote: > > This change adds support for not enabling carrier during TUNSETIFF > interface creation by specifying the IFF_NO_CARRIER flag. > > Our tests make heavy use of tun interfaces. In some scenarios, the test > process creates the interface but another process brings it up after the > interface is discovered via netlink notification. In that case, it is > not possible to create a tun/tap interface with carrier off without it > racing against the bring up. Immediately setting carrier off via > TUNSETCARRIER is still too late. > > Signed-off-by: Patrick Rohr <prohr@google.com> > Cc: Maciej Żenczykowski <maze@google.com> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Stephen Hemminger <stephen@networkplumber.org> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > drivers/net/tun.c | 9 ++++++--- > include/uapi/linux/if_tun.h | 2 ++ > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 259b2b84b2b3..db736b944016 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2828,7 +2828,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) > rcu_assign_pointer(tfile->tun, tun); > } > > - netif_carrier_on(tun->dev); > + if (ifr->ifr_flags & IFF_NO_CARRIER) > + netif_carrier_off(tun->dev); > + else > + netif_carrier_on(tun->dev); > > /* Make sure persistent devices do not get stuck in > * xoff state. > @@ -3056,8 +3059,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > * This is needed because we never checked for invalid flags on > * TUNSETIFF. > */ > - return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, > - (unsigned int __user*)argp); > + return put_user(IFF_TUN | IFF_TAP | IFF_NO_CARRIER | > + TUN_FEATURES, (unsigned int __user*)argp); > } else if (cmd == TUNSETQUEUE) { > return tun_set_queue(file, &ifr); > } else if (cmd == SIOCGSKNS) { > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h > index 2ec07de1d73b..b6d7b868f290 100644 > --- a/include/uapi/linux/if_tun.h > +++ b/include/uapi/linux/if_tun.h > @@ -67,6 +67,8 @@ > #define IFF_TAP 0x0002 > #define IFF_NAPI 0x0010 > #define IFF_NAPI_FRAGS 0x0020 > +/* Used in TUNSETIFF to bring up tun/tap without carrier */ > +#define IFF_NO_CARRIER 0x0040 > #define IFF_NO_PI 0x1000 > /* This flag has no real effect */ > #define IFF_ONE_QUEUE 0x2000 > -- > 2.37.3.968.ga6b4b080e4-goog Reviewed-by: Maciej Żenczykowski <maze@google.com> Ideally we'd get this into LTS trees as well, but I'm failing to think of an appropriate Fixes tag to make that automatically just happen... since this isn't really a fix per-say... So I guess we'll have to either request stable@ to pull it into LTS, or manually cherrypick into all Android Common Kernel trees (4.14+ I guess). Additionally, we talked this over in person, and it appears that storing the IFF_NO_CARRIER in tun->ifr.ifr_flags is a non-issue, because this field is only ever used from tun_net_init() where it gets masked out anyway. That said, the existence of the tun->ifr field seems like unnecessary complexity and we should probably refactor this out afterwards.
On Wed, Sep 21, 2022 at 3:48 AM Patrick Rohr <prohr@google.com> wrote: > > This change adds support for not enabling carrier during TUNSETIFF > interface creation by specifying the IFF_NO_CARRIER flag. > > Our tests make heavy use of tun interfaces. In some scenarios, the test > process creates the interface but another process brings it up after the > interface is discovered via netlink notification. In that case, it is > not possible to create a tun/tap interface with carrier off without it > racing against the bring up. Immediately setting carrier off via > TUNSETCARRIER is still too late. > > Signed-off-by: Patrick Rohr <prohr@google.com> > Cc: Maciej Żenczykowski <maze@google.com> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Stephen Hemminger <stephen@networkplumber.org> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > drivers/net/tun.c | 9 ++++++--- > include/uapi/linux/if_tun.h | 2 ++ > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 259b2b84b2b3..db736b944016 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2828,7 +2828,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) > rcu_assign_pointer(tfile->tun, tun); > } > > - netif_carrier_on(tun->dev); > + if (ifr->ifr_flags & IFF_NO_CARRIER) > + netif_carrier_off(tun->dev); > + else > + netif_carrier_on(tun->dev); > > /* Make sure persistent devices do not get stuck in > * xoff state. > @@ -3056,8 +3059,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > * This is needed because we never checked for invalid flags on > * TUNSETIFF. > */ > - return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, > - (unsigned int __user*)argp); > + return put_user(IFF_TUN | IFF_TAP | IFF_NO_CARRIER | > + TUN_FEATURES, (unsigned int __user*)argp); > } else if (cmd == TUNSETQUEUE) { > return tun_set_queue(file, &ifr); > } else if (cmd == SIOCGSKNS) { > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h > index 2ec07de1d73b..b6d7b868f290 100644 > --- a/include/uapi/linux/if_tun.h > +++ b/include/uapi/linux/if_tun.h > @@ -67,6 +67,8 @@ > #define IFF_TAP 0x0002 > #define IFF_NAPI 0x0010 > #define IFF_NAPI_FRAGS 0x0020 > +/* Used in TUNSETIFF to bring up tun/tap without carrier */ > +#define IFF_NO_CARRIER 0x0040 > #define IFF_NO_PI 0x1000 > /* This flag has no real effect */ > #define IFF_ONE_QUEUE 0x2000 > -- > 2.37.3.968.ga6b4b080e4-goog >
Le 20/09/2022 à 21:48, Patrick Rohr a écrit : > This change adds support for not enabling carrier during TUNSETIFF > interface creation by specifying the IFF_NO_CARRIER flag. > > Our tests make heavy use of tun interfaces. In some scenarios, the test > process creates the interface but another process brings it up after the > interface is discovered via netlink notification. In that case, it is > not possible to create a tun/tap interface with carrier off without it > racing against the bring up. Immediately setting carrier off via > TUNSETCARRIER is still too late. > > Signed-off-by: Patrick Rohr <prohr@google.com> > Cc: Maciej Żenczykowski <maze@google.com> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Stephen Hemminger <stephen@networkplumber.org> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Tue, 20 Sep 2022 12:48:25 -0700 you wrote: > This change adds support for not enabling carrier during TUNSETIFF > interface creation by specifying the IFF_NO_CARRIER flag. > > Our tests make heavy use of tun interfaces. In some scenarios, the test > process creates the interface but another process brings it up after the > interface is discovered via netlink notification. In that case, it is > not possible to create a tun/tap interface with carrier off without it > racing against the bring up. Immediately setting carrier off via > TUNSETCARRIER is still too late. > > [...] Here is the summary with links: - [v2] tun: support not enabling carrier in TUNSETIFF https://git.kernel.org/netdev/net/c/195624d9c26b You are awesome, thank you!
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 259b2b84b2b3..db736b944016 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -2828,7 +2828,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) rcu_assign_pointer(tfile->tun, tun); } - netif_carrier_on(tun->dev); + if (ifr->ifr_flags & IFF_NO_CARRIER) + netif_carrier_off(tun->dev); + else + netif_carrier_on(tun->dev); /* Make sure persistent devices do not get stuck in * xoff state. @@ -3056,8 +3059,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, * This is needed because we never checked for invalid flags on * TUNSETIFF. */ - return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, - (unsigned int __user*)argp); + return put_user(IFF_TUN | IFF_TAP | IFF_NO_CARRIER | + TUN_FEATURES, (unsigned int __user*)argp); } else if (cmd == TUNSETQUEUE) { return tun_set_queue(file, &ifr); } else if (cmd == SIOCGSKNS) { diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h index 2ec07de1d73b..b6d7b868f290 100644 --- a/include/uapi/linux/if_tun.h +++ b/include/uapi/linux/if_tun.h @@ -67,6 +67,8 @@ #define IFF_TAP 0x0002 #define IFF_NAPI 0x0010 #define IFF_NAPI_FRAGS 0x0020 +/* Used in TUNSETIFF to bring up tun/tap without carrier */ +#define IFF_NO_CARRIER 0x0040 #define IFF_NO_PI 0x1000 /* This flag has no real effect */ #define IFF_ONE_QUEUE 0x2000
This change adds support for not enabling carrier during TUNSETIFF interface creation by specifying the IFF_NO_CARRIER flag. Our tests make heavy use of tun interfaces. In some scenarios, the test process creates the interface but another process brings it up after the interface is discovered via netlink notification. In that case, it is not possible to create a tun/tap interface with carrier off without it racing against the bring up. Immediately setting carrier off via TUNSETCARRIER is still too late. Signed-off-by: Patrick Rohr <prohr@google.com> Cc: Maciej Żenczykowski <maze@google.com> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Jason Wang <jasowang@redhat.com> Cc: Stephen Hemminger <stephen@networkplumber.org> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- drivers/net/tun.c | 9 ++++++--- include/uapi/linux/if_tun.h | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-)