Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

On 14/03/23, David Miller wrote: > From: Richard Guy Briggs <rgb(a)redhat.com&gt; > Date: Fri, 21 Mar 2014 12:39:11 -0400 > > > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, > > if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0])) > > return 0; > > > > + if (nlk->netlink_bind && nladdr->nl_groups) { > > + int i; > > + > > + for (i = 0; i < nlk->ngroups; i++) > > + if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) { > > + err = nlk->netlink_bind(i); > > + if (err) > > + return err; > > + } > > + } > > + > > You can't just leave a partially set of completed bindings in place. In the general case, I agree. > It's not valid to leave half-baked state like this. In the one existing case (netfilter), it adds a module that is never unloaded. (refcounts are bumped up and down, but I don't see an auto-reap based on cleared multicast group subscriptions.) For that matter, netlink_realloc_groups() isn't reversed on error either.

In the proposed case (audit) it is only a permissions check, so there is nothing to undo. So, I was being lazy looking at the existing situation. > If you return an error, all of the binding state changes must be > completely undone. Is it time to add a ".unbind = netlink_unbind" to struct proto_ops netlink_ops? (I am only half serious here...)

> If you can't find a way to do this cleanly, you'll need to find > a way for the audit code to not return an error. Fair enough. I'll go back and look at updating subscriptions and listeners first and undoing those actions if the bind fails. In the case of netlink_setsockopt() it is just one to undo, which is easy. netlink_bind() is a bit more complex, but doable. The whole purpose here was to add a way for each protocol to be able to add its own permissions check and signal a way for netlink to refuse the subscription if the userspace process doesn't have the required permissions, so not returning an error defeats that whole purpose. - RGB