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?
thanks,
--
davide
(*) I'm planning to write a small module to add support for 'ss'
diagnostics on AF_CAN sockets. This patch was sort-of preparatory work
for kselftests :)
