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: <20220408115054.7471233b@kernel.org>
Date:   Fri, 8 Apr 2022 11:50:54 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
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, 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.