On 17.03.25 16:50, Davide Caratti wrote:
hi Oliver,
On Mon, Mar 17, 2025 at 11:21:25AM +0100, Oliver Hartkopp wrote:
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.
well, it does not even need to be the very last caller. Just +1 for each
socket created, -1 for each socket released. /proc/net/protocols counts the
number of active sockets for each protocol, so it's ok not to account for
dead / already-orphaned ones that are just waiting for being freed in some
later RCU callback - like it happens See below:
[...]
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.
AFAIU sock_put() in can_rx_delete_receiver() is always balanced with
sock_hold() here:
564 /* schedule the receiver item for deletion */
565 if (rcv) {
566 if (rcv->sk)
567 sock_hold(rcv->sk);
568 call_rcu(&rcv->rcu, can_rx_delete_receiver);
569 }
and we have can_rx_unregister() also in ->release(). So, I think it's OK not to
do sock_prot_inuse_add(..., -1) inside the RCU callback and just decrement the
after socket is orphaned. For similar reason, I think that
252 static void __j1939_session_drop(struct j1939_session *session)
253 {
254 if (!session->transmission)
255 return;
256
257 j1939_sock_pending_del(session->sk);
258 sock_put(session->sk);
259 }
does not need to touch 'in_use' counter, because it couples with this:
2025 /* skb is recounted in j1939_session_new() */
2026 sock_hold(skb->sk);
2027 session->sk = skb->sk;
for a socket (namely skb->sk) that's already created.
[...]
this is '->release()' of each protocol, that I aimed to cover in the
patch...
ACK
j1939/socket.c: sock_put(sk);
bottom line, I think we only need one sock_prot_inuse_add(..., -1)
in the above file. WDYT?
Makes sense.
@Marc: Can you please finally review the fix?
Thanks,
Oliver
