netdev - Re: [PATCH net 1/4] net-sysfs: check device is present when showing carrier

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: <c20dcbc18af57f235974c9e5503491ea07a3ce99.camel@sipsolutions.net>
Date: Wed, 24 Jul 2024 11:35:52 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Jamie Bainbridge <jamie.bainbridge@...il.com>, "netdev@...r.kernel.org"
 <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>
Cc: Jiri Pirko <jiri@...nulli.us>, "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 1/4] net-sysfs: check device is present when showing
 carrier

On Wed, 2024-07-24 at 01:46 +0000, Jamie Bainbridge wrote:
> A sysfs reader can race with a device reset or removal.

Kind of, yes, but please check what the race actually is.

> This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add
> check for netdevice being present to speed_show") so add the same check
> to carrier_show.

You didn't say why it's needed here, so ... why is it?

FWIW, I don't think it actually _is_ needed, since the netdev struct
itself is still around, linkwatch_sync_dev() will not do anything that's
not still needed anyway (the removal from list must clearly either still
happen or nothing happens in the function). This will not call into the
driver (which would be the problematic part).

So while I don't think this is _wrong_ per se, I also don't think it's
necessary, nor are you demonstrating that it is.

And for userspace it should be pretty much immaterial whether it gets a
real value or -EINVAL in the race, or -ENOENT because the file
disappeared anyway?

johannes