netdev - Re: RE: [Intel-wired-lan] [PATCH v1 iwl-net] igc: unregister netdev when igc_led_setup() fails in igc

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] [day] [month] [year] [list]

Message-ID: <20250910092537.30823-1-enjuk@amazon.com>
Date: Wed, 10 Sep 2025 18:25:08 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <aleksandr.loktionov@...el.com>
CC: <andrew+netdev@...n.ch>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <enjuk@...zon.com>,
	<intel-wired-lan@...ts.osuosl.org>, <kohei.enju@...il.com>,
	<kuba@...nel.org>, <kurt@...utronix.de>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <przemyslaw.kitszel@...el.com>,
	<vitaly.lifshits@...el.com>
Subject: Re: RE: [Intel-wired-lan] [PATCH v1 iwl-net] igc: unregister netdev when igc_led_setup() fails in igc_probe()

On Wed, 10 Sep 2025 09:02:51 +0000, Loktionov, Aleksandr wrote:

[...]
>> >>
>> >> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
>> >
>> >Thank you for the patch and for identifying this issue!
>> >
>> >I was wondering whether we could avoid failing the probe in cases
>> where
>> >igc_led_setup fails. It seems to me that a failure in the LED class
>> >functionality shouldn't prevent the device's core functionality from
>> >working properly.
>> 
>> Indeed, that also makes sense.
>> 
>> The behavior that igc_probe() succeeds even if igc_led_setup() fails
>> also seems good to me, as long as notifying users that igc's led
>> functionality is not available.
>> 
>> >
>> > From what I understand, errors in this function are not due to
>> hardware
>> >malfunctions. Therefore, I suggest we remove the error propagation.
>> >
>> >Alternatively, if feasible, we could consider reordering the function
>> >calls so that the LED class setup occurs before the netdev
>> registration.
>> >
>> 
>> I don't disagree with you, but I would like to hear Kurt and
>> Aleksandr's
>> opinion. Do you have any preference or suggestions?
>> 
>> I'll revise and work on v2 if needed.
>> Thanks!
>
>Just in case /*I'm Alex*/ here are my 2cents:
>  I’d treat LED setup as best‑effort and not fail probe if it errors.
>Warn once, mark LEDs unavailable, and continue. That keeps datapath
>up and avoids tricky probe unwind. If we still want to fail on LED errors,
>then either (a) fix the unwind (unregister_netdev et al.) or (b) move LED setup before register_netdev().

Got it, thank you for your opinion :)

>
>  If LED labels depend on the netdev name, it’s fine to run LED setup after register_netdev().
>Since errors are non‑fatal, there’s no unwind complexity.
>
>Keep igc_led_setup() returning an error for internal visibility, but don’t propagate it as probe failure:
>err = igc_led_setup(adapter);
>if (err) {
>    netdev_warn_once(netdev,
>                     "LED init failed (%d); continuing without LED support\n",
>                     err);
>    adapter->leds_available = false;
>} else {
>    adapter->leds_available = true;
>}
>
>In remove()/error paths, guard teardown:
>if (adapter->leds_available)
>    igc_led_teardown(adapter);

I would like to adopt this approach, where we don't propagate led's
failure and manage its status using flag like leds_available.

I'll send v2 shortly.

>
>Keep current order but fully unwind on error:
>err = igc_led_setup(adapter);
>if (err) {
>    unregister_netdev(netdev);
>    /* del NAPI, free queues, etc. in reverse order */
>    err = -E...;
>    goto err_free;
>}
>
>With the best regards
>Alex