netdev - Re: [PATCH net] r8169: fix LED-related deadlock on module removal

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: <ZhzqB9_xvEKSkMB7@wunner.de>
Date: Mon, 15 Apr 2024 10:49:11 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] r8169: fix LED-related deadlock on module removal

On Mon, Apr 15, 2024 at 08:44:35AM +0200, Heiner Kallweit wrote:
> Binding devm_led_classdev_register() to the netdev is problematic
> because on module removal we get a RTNL-related deadlock.

More precisely the issue is triggered on driver unbind.

Module unload as well as device unplug imply driver unbinding.


> The original change was introduced with 6.8, 6.9 added support for
> LEDs on RTL8125. Therefore the first version of the fix applied on
> 6.9-rc only. This is the modified version for 6.8.

I guess the recipient of this patch should have been the stable
maintainers then, not netdev maintainers.


> --- a/drivers/net/ethernet/realtek/r8169.h
> +++ b/drivers/net/ethernet/realtek/r8169.h
> @@ -72,6 +72,7 @@ enum mac_version {
>  };
>  
>  struct rtl8169_private;
> +struct r8169_led_classdev;

Normally these forward declarations are not needed if you're just
referencing the struct name in a pointer.  Usage of the struct name
in a pointer implies a forward declaration.


> +struct r8169_led_classdev *rtl8168_init_leds(struct net_device *ndev)
>  {
> -	/* bind resource mgmt to netdev */
> -	struct device *dev = &ndev->dev;
>  	struct r8169_led_classdev *leds;
>  	int i;
>  
> -	leds = devm_kcalloc(dev, RTL8168_NUM_LEDS, sizeof(*leds), GFP_KERNEL);
> +	leds = kcalloc(RTL8168_NUM_LEDS + 1, sizeof(*leds), GFP_KERNEL);
>  	if (!leds)
> -		return;
> +		return NULL;
>  
>  	for (i = 0; i < RTL8168_NUM_LEDS; i++)
>  		rtl8168_setup_ldev(leds + i, ndev, i);
> +
> +	return leds;
> +}

If registration of some LEDs fails, you seem to continue driver binding.
So the leds allocation may stick around even if it's not used at all.
Not a big deal, but not super pretty either.

Thanks,

Lukas