Hi Davide,
On 17.03.25 11:14, Davide Caratti wrote:
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):
Right.
[...]
~/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;
Right. That's how I understood it too.
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.
The can_rx_delete_receiver() might be called if the (e.g. USB) CAN
interface is removed in the network notifier. So this is no gracefully
socket termination from user space.
I think the need to decrease the prot-in-use counter here too.
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...
ACK
j1939/socket.c: sock_put(sk);
j1939/transport.c: sock_put(session->sk);
... except this one, that I forgot :)
Things happen ;)
I will send a follow-up patch soon.
Thanks!
Thanks!
Best regards,
Oliver
