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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 20 Sep 2022 09:44:45 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On Tue, Sep 20, 2022 at 8:01 AM Maciej Żenczykowski <maze@google.com> wrote:
> >
> > On Mon, Sep 19, 2022 at 10:18 AM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:  
> > > On Fri, 16 Sep 2022 16:45:52 -0700
> > > Patrick Rohr <prohr@google.com> wrote:  
> > > >  #define IFF_DETACH_QUEUE 0x0400
> > > > +/* Used in TUNSETIFF to bring up tun/tap without carrier */
> > > > +#define IFF_NO_CARRIER IFF_DETACH_QUEUE  
> > >
> > > Overloading a flag in existing user API is likely to break
> > > some application somewhere...  
> >
> > We could of course burn a bit (0x0040 and 0x0080 are both currently
> > utterly unused)... but that just seemed wasteful...
> > Do you think that would be better?
> >
> > I find it exceedingly unlikely that any application is specifying this
> > flag to TUNSETIFF currently.
> >
> > This flag has barely any hits in the code base, indeed ignoring the
> > Documentation, tests, and #define's we have:
> >
> > $ git grep IFF_DETACH_QUEUE
> > drivers/net/tap.c:928:  else if (flags & IFF_DETACH_QUEUE)
> > drivers/net/tun.c:2954: } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> > drivers/net/tun.c:3115:                 ifr.ifr_flags |= IFF_DETACH_QUEUE;
> >
> > The first two implement ioctl(TUNSETQUEUE) -- that's the only spot
> > where IFF_DETACH_QUEUE is currently supposed to be used.
> >
> > The third one is the most interesting, see drivers/net/tun.c:3111
> >
> >  case TUNGETIFF:
> >          tun_get_iff(tun, &ifr);
> >          if (tfile->detached)
> >                  ifr.ifr_flags |= IFF_DETACH_QUEUE;
> >          if (!tfile->socket.sk->sk_filter)
> >                  ifr.ifr_flags |= IFF_NOFILTER;
> >
> > This means TUNGETIFF can return this flag for a detached queue.  However:
> >
> > (a) multiqueue tun/tap is pretty niche, and detached queues are even more niche.
> >
> > (b) the TUNGETIFF returned ifr_flags field already cannot be safely
> > used as input to TUNSETIFF,  
> 
> Yes, but it could be used by userspace to recover the multiqueue state
> via TUNSETQUEUE for a feature like checkpoint.
> 
> > because IFF_NOFILTER == IFF_NO_PI ==
> > 0x1000
> >
> > (this overlap of IFF_NO_PI and IFF_NOFILTER is why we thought it'd be
> > ok to overlap here as well)
> >
> > (c) if this actually turns out to be a problem it shouldn't be that
> > hard to fix the 1 or 2 userspace programs to mask out the flag
> > and not pass in garbage... Do we really want / need to maintain
> > compatibility with extremely badly written userspace?  
> 
> Not sure, but instead of trying to answer this hard question, having a
> new flag seems to be easier.
> 
> > It's really hard to even imagine how such code would come into existence...
> >
> > Arguably the TUNSETIFF api should have always returned an error for
> > invalid flags... should we make that change now?  
> 
> Probably too late to do that.

There have been several other cases where Linus has said
ABI compatability includes not breaking buggy userspace applications.
Look at the history around new syscalls that add a flag argument
and forget to check that it is zero in the first version.