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: <20220411153334.lpzilb57wddxlzml@skbuf>
Date:   Mon, 11 Apr 2022 18:33:34 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: What is the purpose of dev->gflags?

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?