Re: [PATCH net-next v2] hsr: broadcast netlink notifications in the device's net namespace

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 6/2/26 9:06 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> hsr: broadcast netlink notifications in the device's net namespace
> 
> This commit modifies the HSR subsystem to broadcast generic netlink
> notifications in the device's network namespace instead of init_net. It updates
> hsr_nl_ringerror() and hsr_nl_nodedown() to use genlmsg_multicast_netns().
> 
>> diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
>> index f0ca23da3ab99..eb6d222535d8b 100644
>> --- a/net/hsr/hsr_netlink.c
>> +++ b/net/hsr/hsr_netlink.c
> [ ... ]
>> @@ -288,8 +289,17 @@ void hsr_nl_nodedown(struct hsr_priv *hsr, unsigned char addr[ETH_ALEN])
>>   	if (res < 0)
>>   		goto nla_put_failure;
>>   
>> +	rcu_read_lock();
>> +	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
>> +	if (!master) {
>> +		rcu_read_unlock();
>> +		goto nla_put_failure;
> 
> [Severity: High]
> Does jumping to nla_put_failure here cause a NULL pointer dereference?
> 
> If master is NULL, the code jumps to nla_put_failure, which frees the skb
> and then falls through to the fail label:
> 
> nla_put_failure:
> 	kfree_skb(skb);
> 
> fail:
> 	rcu_read_lock();
> 	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> 	netdev_warn(master->dev, "Could not send HSR node down\n");
> 	rcu_read_unlock();
> 
> Since master was not found previously, the re-fetch will likely still be NULL,
> meaning the dereference of master->dev in netdev_warn() will crash.
> 

Hi Jakub,

FWIW I don't think this can happen. I didn't mention it because it was 
kind of pointless but I was wrong.

i.e master cannot be NULL here. When cleaning up the link at 
hsr_dellink() we always call timer_delete_sync() for all the background 
jobs before removing the master.

But given this AI report it seems it is just better to remove the master 
check that this patch is introducing to avoid AI reports all over the code.

Thanks,
Fernando.

>> +	}
>> +
>>   	genlmsg_end(skb, msg_head);
>> -	genlmsg_multicast(&hsr_genl_family, skb, 0, 0, GFP_ATOMIC);
>> +	genlmsg_multicast_netns(&hsr_genl_family, dev_net(master->dev),
>> +				skb, 0, 0, GFP_ATOMIC);
>> +	rcu_read_unlock();
>>   
>>   	return;
>>