netdev - Re: [PATCH v2] tun: support not enabling carrier in TUNSETIFF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANP3RGewDp-ofCi72zskEZ2VjXotQR+C=aiVSt1=a=cL9Xd1=Q@mail.gmail.com>
Date:   Tue, 20 Sep 2022 14:47:54 -0700
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Patrick Rohr <prohr@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>, Lorenzo Colitti <lorenzo@...gle.com>,
        Jason Wang <jasowang@...hat.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>
Subject: Re: [PATCH v2] tun: support not enabling carrier in TUNSETIFF

On Tue, Sep 20, 2022 at 12:48 PM Patrick Rohr <prohr@...gle.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@...gle.com>
> Cc: Maciej Żenczykowski <maze@...gle.com>
> Cc: Lorenzo Colitti <lorenzo@...gle.com>
> Cc: Jason Wang <jasowang@...hat.com>
> Cc: Stephen Hemminger <stephen@...workplumber.org>
> Cc: Nicolas Dichtel <nicolas.dichtel@...nd.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@...gle.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.