thanks for reviewing!
On Sun, Mar 16, 2025 at 01:36:40PM +0100, Oliver Hartkopp wrote:
> Hello Davide,
>
> thanks for your patch!
ouch, I forgot j1939. I had a selftest for that, but I could only
test raw.
> But don't we need to take care on every place where sock_put() is called
> where sock_prot_inuse_add() has to decrease the counter?
only the last call to sock_put() needs sock_prot_inuse_add(..., -1):
[...]
> ~/linux/net/can$ git grep sock_put .
> af_can.c: sock_put(sk);
167 if (sk->sk_prot->init)
168 err = sk->sk_prot->init(sk);
169
170 if (err) {
171 /* release sk on errors */
172 sock_orphan(sk);
173 sock_put(sk);
174 sock->sk = NULL;
175 } else {
176 sock_prot_inuse_add(net, sk->sk_prot, 1);
177 }
^^ this one does not need it because the 'in_use' counter is incremented
in the else branch;
> af_can.c: sock_put(sk);
491 static void can_rx_delete_receiver(struct rcu_head *rp)
492 {
493 struct receiver *rcv = container_of(rp, struct receiver, rcu);
494 struct sock *sk = rcv->sk;
495
496 kmem_cache_free(rcv_cache, rcv);
497 if (sk)
498 sock_put(sk);
499 }
this one comes from can_rx_unregister(), and it's called in RCU callback - so
we can't tell if it happens before or after sock_put() in ->release().
So we probably need something smarter in case we are not sure that ->release()
is called at least once for each socket.
> bcm.c: sock_put(sk);
> isotp.c: sock_put(sk);
> raw.c: sock_put(sk);
this is '->release()' of each protocol, that I aimed to cover in the
patch...
> j1939/socket.c: sock_put(sk);
> j1939/transport.c: sock_put(session->sk);
... except this one, that I forgot :)
I will send a follow-up patch soon.
Thanks!
--
davide
