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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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