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

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: <797f525b-9b85-9f86-2927-6dfb34e61c31@6wind.com>
Date:   Mon, 11 Apr 2022 17:26:05 +0200
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: What is the purpose of dev->gflags?



Le 08/04/2022 à 21:17, Vladimir Oltean a écrit :
> 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? ]
I think this was historical, I had the same questions a long time ago.

>>
>> 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 ]).
If I remember well, the goal was to advertise these flags to userspace only when
they were set by a userspace app and not by a kernel module (bridge, bonding, etc).
To avoid changing that behavior, IFLA_PROMISCUITY was introduced, thus userspace
may know if promiscuity is enabled by dumping the interface. Notifications were
fixed later, but maybe some are still missing.


Regards,
Nicolas


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