netdev - Re: [PATCH wireless-next 0/3] netlink carrier race workaround

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: <d918321ab519800018a9fed71eb8e43d64cd934a.camel@sipsolutions.net>
Date: Mon, 04 Dec 2023 20:14:10 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH wireless-next 0/3] netlink carrier race workaround

On Mon, 2023-12-04 at 08:23 -0800, Jakub Kicinski wrote:
> On Sun, 03 Dec 2023 19:51:28 +0100 Johannes Berg wrote:
> > I think I wouldn't mind now, and perhaps if we want to sync in netlink
> > we should also do this here so that it's consistent, but I'm not sure
> > I'd want this to be the only way to do it, I might imagine that someone
> > might want this in some kind of container that doesn't necessarily have
> > (full) access there? Dunno.
> 
> Also dunno :) We can add a "sync" version of netif_carrier_ok()
> and then call if from whatever places we need.

[note: netif_carrier_ok(), not netif_carrier_on(), almost confused that]

Yeah I guess we can have a netif_carrier_ok_sync(), though it feels kind
of dubious to hide such an important detail in the middle of a netlink
message building:

        if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
...
#ifdef CONFIG_RPS
            nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
#endif
            put_master_ifindex(skb, dev) ||
            nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
...

Also, if we ever _do_ want to make it optional, then it's problematic,
do we do netif_carrier_ok_maybe_sync(dev, sync)? ;-)

> > We _could_ also use an input attribute on the rtnl_getlink() call to
> > have userspace explicitly opt in to doing the sync before returning
> > information?
> 
> Yeah, maybe.. IMHO a more "Rusty Russell API levels" thing to do would
> be to allow opting out, as those who set the magic flag "know what they
> are doing" and returning unsync'ed carrier may be surprising.
> Also a "don't sync flag" we can add later, once someone who actually
> cares appears, avoiding uAPI growth 😁️
> 
> Anyway, up to you :)

Heh. But do I want to get blamed for the (perhaps inevitable?)
performance regression? I guess I'll try ...


Actually I could even still combine this with the netif carrier up count
in the wireless events, so we only have to do the rtnl_getlink if we
haven't seen an event yet, and - in the likely common case - save the
extra roundtrip? Though I guess it's not a huge problem, it's once per
connection basically.

johannes