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: <bb0b2488-d5f7-4530-ad30-06b30f064fa7@gmail.com>
Date: Mon, 15 Apr 2024 13:54:54 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Lukas Wunner <lukas@...ner.de>
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 15.04.2024 10:49, Lukas Wunner wrote:
> 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.
> 
OK, seems this has changed over time. Following still mentioned
that netdev is special.
https://www.kernel.org/doc/html/v4.19/process/stable-kernel-rules.html

> 
>> --- 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.
> 
Even if technically not needed, it seems to be kernel best practice
to use forward declarations, see e.g. device.h.
However I'd be interested in hearing the maintainers position to
consider this with the next submissions.

> 
>> +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
>