netdev - Re: Re: [PATCH net] genetlink: fix genl_bind() invoking bind() after -EPERM

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: <8190d77e-5183-465e-bfc8-9c1a9690bdf0@oracle.com>
Date: Mon, 1 Sep 2025 15:04:59 +0530
From: ALOK TIWARI <alok.a.tiwari@...cle.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: jiri@...dia.com, stanislaw.gruszka@...ux.intel.com, andrew+netdev@...n.ch,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, horms@...nel.org, netdev@...r.kernel.org
Subject: Re: Re: [PATCH net] genetlink: fix genl_bind() invoking bind() after
 -EPERM



On 9/1/2025 6:53 AM, Andrew Lunn wrote:
>> Fixes: 3de21a8990d3 ("genetlink: Add per family bind/unbind callbacks")
>> Signed-off-by: Alok Tiwari<alok.a.tiwari@...cle.com>
>> ---
>>   net/netlink/genetlink.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
>> index 104732d34543..3b51fbd068ac 100644
>> --- a/net/netlink/genetlink.c
>> +++ b/net/netlink/genetlink.c
>> @@ -1836,7 +1836,7 @@ static int genl_bind(struct net *net, int group)
>>   		    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
>>   			ret = -EPERM;
>>   
>> -		if (family->bind)
>> +		if (!ret && family->bind)
>>   			family->bind(i);
> I agree, this fixes the issue you point out. But i think it would be
> more robust if after each EPERM there was a continue.
> 
> Also, i don't understand how this ret value is used. It looks like the
> bind() op could be called a number of times, and yet genl_bind()
> returns -EPERM?
> 
> Also, struct genl_family defines bind() as returning an int. It does
> not say so, but i assume the return value is 0 on success, negative
> error code on failure. Should we be throwing this return value away?
> Should genl_bind() return an error code if the bind failed?
> 
> And if genl_bind() does return an error, should it first cleanup and
> unbind any which were successful bound?
> 
> As i said, i don't know this code, so all i can do is ask questions in
> the hope somebody does know what is supposed to happen here.
> 
>         Andrew


Thanks Andrew.

I am still pretty new to netdev and trying to understand these details.
I also had a similar feeling, returning -EPERM directly looks a bit
odd without a continue and I was also wondering how the ret value is 
actually used.

I am positive other maintainers will offer their valuable
suggestions and help clarify these doubts.


Thanks,
Alok