Re: [PATCH] can: add protocol counter for AF_CAN sockets

[Davide Caratti <dcaratti@redhat.com>]

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 :)