Re: What is the purpose of dev->gflags?

[Jakub Kicinski <kuba@kernel.org>]

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? 

> 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.