Re: [PATCH net] can: isotp: check register_netdevice_notifier() error in module init

Hello Minhong He,

thanks for your improvement - but sashiko-bot is right too.

Currently isotp.c is the only CAN module that misses the error handling 
and the rule of thumb to expose the userspace visibility as a last step.

bcm.c and raw.c are already correct in this matter.

Please send a V2 which also references the bcm.c/raw.c init functions:

"Align isotp_module_init() with the reordering already done for raw.c 
(commit …) and bcm.c (commit …)."

Best regards,
Oliver

On 28.07.26 05:20, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Registering the CAN protocol before completing all fallible initialization steps creates a race window leading to a Use-After-Free crash if initialization fails.
> --
> 
> --- Patch [1]: [PATCH net] can: isotp: check register_netdevice_notifier() error in module init ---
> commit 5f2f7a58360e9a2c18b5d6a9a724f468f79227d1
> Author: Minhong He <[email protected]>
> 
> can: isotp: check register_netdevice_notifier() error in module init
> 
> This commit adds a check for the return value of register_netdevice_notifier()
> in the isotp module initialization. If the notifier registration fails, it
> now unregisters the CAN protocol and returns the error.
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 54becaf6898f1..65e890cafd7a9 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -1908,12 +1908,18 @@ static __init int isotp_module_init(void)
>>   	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
>>   
>>   	err = can_proto_register(&isotp_can_proto);
>> -	if (err < 0)
>> +	if (err < 0) {
>>   		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
>> -	else
>> -		register_netdevice_notifier(&canisotp_notifier);
>> +		return err;
>> +	}
>>   
>> -	return err;
>> +	err = register_netdevice_notifier(&canisotp_notifier);
>> +	if (err) {
>> +		can_proto_unregister(&isotp_can_proto);
>> +		return err;
>> +	}
> 
> [Severity: High]
> Does this error path introduce a use-after-free risk if userspace creates a
> socket before the module initialization completes?
> 
> If can_proto_register() succeeds, the protocol is immediately exposed to
> userspace. If a concurrent thread creates an ISOTP socket before
> register_netdevice_notifier() fails, the socket will hold pointers to the
> module's protocol operations.
> 
> When the module initialization aborts and returns an error, the kernel will
> free the module memory. While can_proto_unregister() removes the protocol
> from the registry, it does not revoke already-created sockets. As a result,
> any subsequent operations on the existing socket will dereference freed
> module memory.
> 
> Would it be safer to move the fallible register_netdevice_notifier() before
> can_proto_register() to avoid exposing the protocol before initialization
> is guaranteed to succeed?
>