What is the purpose of dev->gflags?

What is the purpose of dev->gflags?

[Vladimir Oltean]

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? ]

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.

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

[Jakub Kicinski]

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.

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

[Vladimir Oltean]

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

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

[Nicolas Dichtel]

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

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

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 05:26:05PM +0200, Nicolas Dichtel wrote:
> 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.

Thanks.
Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
call path is dead code? If it is, is there any problem it should be
addressing which it isn't, or can we just delete it?

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

[Nicolas Dichtel]

Le 11/04/2022 à 17:33, Vladimir Oltean a écrit :
[snip]
> Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
> call path is dead code? If it is, is there any problem it should be
> addressing which it isn't, or can we just delete it?
I probably miss your point, why is it dead code?

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

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 05:43:01PM +0200, Nicolas Dichtel wrote:
> 
> Le 11/04/2022 à 17:33, Vladimir Oltean a écrit :
> [snip]
> > Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
> > call path is dead code? If it is, is there any problem it should be
> > addressing which it isn't, or can we just delete it?
> I probably miss your point, why is it dead code?

Because __dev_set_allmulti() doesn't update dev->gflags, it means
dev->gflags == old_gflags. In turn, it means dev->gflags ^ old_gflags,
passed to "gchanges" of __dev_notify_flags(), is 0.

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

[Nicolas Dichtel]

Le 11/04/2022 à 17:49, Vladimir Oltean a écrit :
> On Mon, Apr 11, 2022 at 05:43:01PM +0200, Nicolas Dichtel wrote:
>>
>> Le 11/04/2022 à 17:33, Vladimir Oltean a écrit :
>> [snip]
>>> Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
>>> call path is dead code? If it is, is there any problem it should be
>>> addressing which it isn't, or can we just delete it?
>> I probably miss your point, why is it dead code?
> 
> Because __dev_set_allmulti() doesn't update dev->gflags, it means
> dev->gflags == old_gflags. In turn, it means dev->gflags ^ old_gflags,
> passed to "gchanges" of __dev_notify_flags(), is 0.
I didn't take any assumptions on dev->gflags because two functions are called
with dev as parameter (dev_change_rx_flags() and dev_set_rx_mode()).
Even if __dev_notify_flags() is called with 0 for the last arg, it calls
notifiers. Thus, this is not "dead code".

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

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 06:10:49PM +0200, Nicolas Dichtel wrote:
> 
> Le 11/04/2022 à 17:49, Vladimir Oltean a écrit :
> > On Mon, Apr 11, 2022 at 05:43:01PM +0200, Nicolas Dichtel wrote:
> >>
> >> Le 11/04/2022 à 17:33, Vladimir Oltean a écrit :
> >> [snip]
> >>> Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
> >>> call path is dead code? If it is, is there any problem it should be
> >>> addressing which it isn't, or can we just delete it?
> >> I probably miss your point, why is it dead code?
> > 
> > Because __dev_set_allmulti() doesn't update dev->gflags, it means
> > dev->gflags == old_gflags. In turn, it means dev->gflags ^ old_gflags,
> > passed to "gchanges" of __dev_notify_flags(), is 0.
> I didn't take any assumptions on dev->gflags because two functions are called
> with dev as parameter (dev_change_rx_flags() and dev_set_rx_mode()).

You mean ops->ndo_change_rx_flags() or ops->ndo_set_rx_mode() are
expected to update dev->gflags?

> Even if __dev_notify_flags() is called with 0 for the last arg, it calls
> notifiers. Thus, this is not "dead code".

The relevant "changes" (dev->flags & old_flags) of the net_device which
may have changed from __dev_set_allmulti() are masked out from
call_netdevice_notifiers(), are they not?

	if (changes & IFF_UP) {
		/* doesn't apply */
	}

	if (dev->flags & IFF_UP &&
	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
	               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	               these changes are masked out

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

[Nicolas Dichtel]

Le 11/04/2022 à 18:20, Vladimir Oltean a écrit :
> On Mon, Apr 11, 2022 at 06:10:49PM +0200, Nicolas Dichtel wrote:
>>
>> Le 11/04/2022 à 17:49, Vladimir Oltean a écrit :
>>> On Mon, Apr 11, 2022 at 05:43:01PM +0200, Nicolas Dichtel wrote:
>>>>
>>>> Le 11/04/2022 à 17:33, Vladimir Oltean a écrit :
>>>> [snip]
>>>>> Would you agree that the __dev_set_allmulti() -> __dev_notify_flags()
>>>>> call path is dead code? If it is, is there any problem it should be
>>>>> addressing which it isn't, or can we just delete it?
>>>> I probably miss your point, why is it dead code?
>>>
>>> Because __dev_set_allmulti() doesn't update dev->gflags, it means
>>> dev->gflags == old_gflags. In turn, it means dev->gflags ^ old_gflags,
>>> passed to "gchanges" of __dev_notify_flags(), is 0.
>> I didn't take any assumptions on dev->gflags because two functions are called
>> with dev as parameter (dev_change_rx_flags() and dev_set_rx_mode()).
> 
> You mean ops->ndo_change_rx_flags() or ops->ndo_set_rx_mode() are
> expected to update dev->gflags?
No, I just say that I didn't take any assumptions on what there are expected to do.

> 
>> Even if __dev_notify_flags() is called with 0 for the last arg, it calls
>> notifiers. Thus, this is not "dead code".
> 
> The relevant "changes" (dev->flags & old_flags) of the net_device which
> may have changed from __dev_set_allmulti() are masked out from
> call_netdevice_notifiers(), are they not?
> 
> 	if (changes & IFF_UP) {
> 		/* doesn't apply */
> 	}
> 
> 	if (dev->flags & IFF_UP &&
> 	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
> 	               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 	               these changes are masked out
Same here. Some complex path are called (eg. dev_change_rx_flags =>
ops->ndo_change_rx_flags() => vlan_dev_change_rx_flags => dev_set_allmulti =>
__dev_set_allmulti => etc).
Maybe you made an audit to check that other flags cannot be changed. But, if it
changes in the future, we will miss them here.

Did you see a bug? What is the issue?

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

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 06:27:54PM +0200, Nicolas Dichtel wrote:
> Same here. Some complex path are called (eg. dev_change_rx_flags =>
> ops->ndo_change_rx_flags() => vlan_dev_change_rx_flags => dev_set_allmulti =>
> __dev_set_allmulti => etc).
> Maybe you made an audit to check that other flags cannot be changed. But, if it
> changes in the future, we will miss them here.

I guess I just don't see what other dev->flags that aren't masked out
from netdev notifier calls may or should change during the call to
__dev_set_allmulti(), regardless of the complexity or depth of the
call path.

And the commit that added the __dev_notify_flags() call said "dev:
always advertise rx_flags changes via netlink" (i.e. the function was
called for its rtmsg_ifinfo() part, not for its call_netdevice_notifiers()
part).

There *was* no call to dev_notify_flags prior to that commit, and it
didn't give a reason for voluntarily going through the netdev notifiers,
either.

> Did you see a bug? What is the issue?

I didn't see any bug, as mentioned I was trying to follow how
dev->gflags is used (see title) and stumbled upon this strange pattern
while doing so. dev->gflags is not updated from dev_set_allmulti()
almost by definition, otherwise in-kernel drivers wouldn't have a way to
update IFF_ALLMULTI without user space becoming aware of it.

The reason for emailing you to was to understand the intention, I do
understand that the code has went through changes since 2013 and that
a more in-depth audit is still needed before making any change.

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

[Nicolas Dichtel]

Le 11/04/2022 à 18:50, Vladimir Oltean a écrit :
> On Mon, Apr 11, 2022 at 06:27:54PM +0200, Nicolas Dichtel wrote:
>> Same here. Some complex path are called (eg. dev_change_rx_flags =>
>> ops->ndo_change_rx_flags() => vlan_dev_change_rx_flags => dev_set_allmulti =>
>> __dev_set_allmulti => etc).
>> Maybe you made an audit to check that other flags cannot be changed. But, if it
>> changes in the future, we will miss them here.
> 
> I guess I just don't see what other dev->flags that aren't masked out
> from netdev notifier calls may or should change during the call to
> __dev_set_allmulti(), regardless of the complexity or depth of the
> call path.
> 
> And the commit that added the __dev_notify_flags() call said "dev:
> always advertise rx_flags changes via netlink" (i.e. the function was
> called for its rtmsg_ifinfo() part, not for its call_netdevice_notifiers()
> part).
> 
> There *was* no call to dev_notify_flags prior to that commit, and it
> didn't give a reason for voluntarily going through the netdev notifiers,
> either.
Yes.

> 
>> Did you see a bug? What is the issue?
> 
> I didn't see any bug, as mentioned I was trying to follow how
> dev->gflags is used (see title) and stumbled upon this strange pattern
> while doing so. dev->gflags is not updated from dev_set_allmulti()
> almost by definition, otherwise in-kernel drivers wouldn't have a way to
> update IFF_ALLMULTI without user space becoming aware of it.
FWIW, here is the patch that has introduced the gflags field:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/netdev-vger-cvs.git/commit/?id=c7a329628f395

> 
> The reason for emailing you to was to understand the intention, I do
> understand that the code has went through changes since 2013 and that
> a more in-depth audit is still needed before making any change.
Yep, because notifiers are called since this patch and maybe some modules expect
this now.