netdev - Re: [PATCHv2 net] usbnet: fix cyclical race on disconnect with work queue

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: <20240910154405.641a459f@kernel.org>
Date: Tue, 10 Sep 2024 15:44:05 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Oliver Neukum <oneukum@...e.com>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCHv2 net] usbnet: fix cyclical race on disconnect with work
 queue

On Thu,  5 Sep 2024 15:46:50 +0200 Oliver Neukum wrote:
> +static inline bool usbnet_going_away(struct usbnet *ubn)
> +{
> +	smp_mb__before_atomic(); /* against usbnet_mark_going_away() */
> +	return test_bit(EVENT_UNPLUG, &ubn->flags);
> +}
> +
> +static inline void usbnet_mark_going_away(struct usbnet *ubn)
> +{
> +	set_bit(EVENT_UNPLUG, &ubn->flags);
> +	smp_mb__after_atomic(); /* against usbnet_going_away() */
> +}

I have sort of an inverse question to what Paolo asked :)
AFAIU we need the double-cancel because checking the flag and
scheduling are not atomic. But if we do that why the memory
barriers? They make it seem like we're doing something clever
with memory ordering, while really we're just depending on normal
properties of the tasklet/timer/work APIs.

FTR disable_work_sync() would work nicely here but it'd be
a PITA for backports.

Also - is this based on some report or syzbot? I'm a bit tempted
to put this in net-next given how unlikely the race is vs how
commonly used the driver is.
-- 
pw-bot: cr