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

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 1 Sep 2025 03:23:36 +0200 Andrew Lunn wrote:
> > @@ -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?

The loop has a break at the end, there can be only one family that owns
a given mcast group ID. So the structure of the code is fine as is
(with the exception of the bug discussed).

As for the fix, to avoid future problems, I'd add a separate:

		if (ret)
			break;

rather than inserting a check into the bind condition.

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

Good question, I think core checks if the group exists within
genetlink, here we should be more or less guaranteed to find
the family. Would be good to test that tho.

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