[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220408191757.dllq7ztaefdyb4i6@skbuf>
Date: Fri, 8 Apr 2022 22:17:57 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>,
Nicolas Dichtel <nicolas.dichtel@...nd.com>
Subject: Re: What is the purpose of dev->gflags?
On Fri, Apr 08, 2022 at 11:50:54AM -0700, Jakub Kicinski wrote:
> On Fri, 8 Apr 2022 21:30:45 +0300 Vladimir Oltean wrote:
> > Hello,
> >
> > I am trying to understand why dev->gflags, which holds a mask of
> > IFF_PROMISC | IFF_ALLMULTI, exists independently of dev->flags.
> >
> > I do see that __dev_change_flags() (called from the ioctl/rtnetlink/sysfs
> > code paths) updates the IFF_PROMISC and IFF_ALLMULTI bits of
> > dev->gflags, while the direct calls to dev_set_promiscuity()/
> > dev_set_allmulti() don't.
> >
> > So at first I'd be tempted to say: IFF_PROMISC | IFF_ALLMULTI are
> > exposed to user space when set in dev->gflags, hidden otherwise.
> > This would be consistent with the implementation of dev_get_flags().
> >
> > [ side note: why is that even desirable? why does it matter who made an
> > interface promiscuous as long as it's promiscuous? ]
>
> Isn't that just a mechanism to make sure user space gets one "refcount"
> on PROMISC and ALLMULTI, while in-kernel calls are tracked individually
> in dev->promiscuity? User space can request promisc while say bridge
> already put ifc into promisc mode, in that case we want promisc to stay
> up even if ifc is unbridged. But setting promisc from user space
> multiple times has no effect, since clear with remove it. Does that
> help?
Yes, that helps to explain one side of it, thanks. But I guess I'm still
confused as to why should a promiscuity setting incremented by the
bridge be invisible to callers of dev_get_flags (SIOCGIFFLAGS,
ifinfomsg::ifi_flags [ *not* IFLA_PROMISCUITY ]).
> > But in the process of digging deeper I stumbled upon Nicolas' commit
> > 991fb3f74c14 ("dev: always advertise rx_flags changes via netlink")
> > which I am still struggling to understand.
> >
> > There, a call to __dev_notify_flags(gchanges=IFF_PROMISC) was added to
> > __dev_set_promiscuity(), called with "notify=true" from dev_set_promiscuity().
> > In my understanding, "gchanges" means "changes to gflags", i.e. to what
> > user space should know about. But as discussed above, direct calls to
> > dev_set_promiscuity() don't update dev->gflags, yet user space is
> > notified via rtmsg_ifinfo() of the promiscuity change.
> >
> > Another oddity with Nicolas' commit: the other added call to
> > __dev_notify_flags(), this time from __dev_set_allmulti().
> > The logic is:
> >
> > static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify)
> > {
> > unsigned int old_flags = dev->flags, old_gflags = dev->gflags;
> >
> > dev->flags |= IFF_ALLMULTI;
> >
> > (bla bla, stuff that doesn't modify dev->gflags)
> >
> > if (dev->flags ^ old_flags) {
> >
> > (bla bla, more stuff that doesn't modify dev->gflags)
> >
> > if (notify)
> > __dev_notify_flags(dev, old_flags,
> > dev->gflags ^ old_gflags);
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> > oops, dev->gflags was never
> > modified, so this call to
> > __dev_notify_flags() is
> > effectively dead code, since
> > user space is not notified,
> > and a NETDEV_CHANGE netdev
> > notifier isn't emitted
> > either, since IFF_ALLMULTI is
> > excluded from that
> > }
> > return 0;
> > }
> >
> > Can someone please clarify what is at least the intention? As can be
> > seen I'm highly confused.
> >
> > Thanks.
>
Powered by blists - more mailing lists