[PATCH] can: add protocol counter for AF_CAN sockets

[PATCH] can: add protocol counter for AF_CAN sockets

[Davide Caratti]

The third column in the output of the following command:

 # grep CAN /proc/net/protocols

is systematically '0': use sock_prot_inuse_add() to account for the number
of sockets for each protocol on top of AF_CAN family.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/can/af_can.c | 2 ++
 net/can/bcm.c    | 1 +
 net/can/isotp.c  | 1 +
 net/can/raw.c    | 5 ++++-
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 01f3fbb3b67d..7b191dbe3693 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -172,6 +172,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
 		sock_orphan(sk);
 		sock_put(sk);
 		sock->sk = NULL;
+	} else {
+		sock_prot_inuse_add(net, sk->sk_prot, 1);
 	}
 
  errout:
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 217049fa496e..6dc041e054ba 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1625,6 +1625,7 @@ static int bcm_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
+	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
 	return 0;
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 16046931542a..789583c62f98 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1239,6 +1239,7 @@ static int isotp_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
+	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
 	return 0;
diff --git a/net/can/raw.c b/net/can/raw.c
index 9b1d5f036f57..020f21430b1d 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -397,11 +397,13 @@ static int raw_release(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
 	struct raw_sock *ro;
+	struct net *net;
 
 	if (!sk)
 		return 0;
 
 	ro = raw_sk(sk);
+	net = sock_net(sk);
 
 	spin_lock(&raw_notifier_lock);
 	while (raw_busy_notifier == ro) {
@@ -421,7 +423,7 @@ static int raw_release(struct socket *sock)
 			raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
 			netdev_put(ro->dev, &ro->dev_tracker);
 		} else {
-			raw_disable_allfilters(sock_net(sk), NULL, sk);
+			raw_disable_allfilters(net, NULL, sk);
 		}
 	}
 
@@ -440,6 +442,7 @@ static int raw_release(struct socket *sock)
 	release_sock(sk);
 	rtnl_unlock();
 
+	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
 	return 0;
-- 
2.47.0

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On 14.03.2025 12:39:49, Davide Caratti wrote:
> The third column in the output of the following command:
> 
>  # grep CAN /proc/net/protocols
> 
> is systematically '0': use sock_prot_inuse_add() to account for the number
> of sockets for each protocol on top of AF_CAN family.
> 
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>

Applied to linux-can-next.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oliver Hartkopp]

Hello Davide,

thanks for your patch!

~/linux/net/can$ git grep sock_put .
af_can.c:               sock_put(sk);
af_can.c:               sock_put(sk);
bcm.c:  sock_put(sk);
isotp.c:        sock_put(sk);
j1939/socket.c: sock_put(sk);
j1939/transport.c:      sock_put(session->sk);
raw.c:  sock_put(sk);

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?

Best regards,
Oliver

On 14.03.25 12:39, Davide Caratti wrote:
> The third column in the output of the following command:
> 
>   # grep CAN /proc/net/protocols
> 
> is systematically '0': use sock_prot_inuse_add() to account for the number
> of sockets for each protocol on top of AF_CAN family.
> 
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>   net/can/af_can.c | 2 ++
>   net/can/bcm.c    | 1 +
>   net/can/isotp.c  | 1 +
>   net/can/raw.c    | 5 ++++-
>   4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 01f3fbb3b67d..7b191dbe3693 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -172,6 +172,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>   		sock_orphan(sk);
>   		sock_put(sk);
>   		sock->sk = NULL;
> +	} else {
> +		sock_prot_inuse_add(net, sk->sk_prot, 1);
>   	}
>   
>    errout:
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 217049fa496e..6dc041e054ba 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1625,6 +1625,7 @@ static int bcm_release(struct socket *sock)
>   	sock->sk = NULL;
>   
>   	release_sock(sk);
> +	sock_prot_inuse_add(net, sk->sk_prot, -1);
>   	sock_put(sk);
>   
>   	return 0;
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 16046931542a..789583c62f98 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1239,6 +1239,7 @@ static int isotp_release(struct socket *sock)
>   	sock->sk = NULL;
>   
>   	release_sock(sk);
> +	sock_prot_inuse_add(net, sk->sk_prot, -1);
>   	sock_put(sk);
>   
>   	return 0;
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 9b1d5f036f57..020f21430b1d 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -397,11 +397,13 @@ static int raw_release(struct socket *sock)
>   {
>   	struct sock *sk = sock->sk;
>   	struct raw_sock *ro;
> +	struct net *net;
>   
>   	if (!sk)
>   		return 0;
>   
>   	ro = raw_sk(sk);
> +	net = sock_net(sk);
>   
>   	spin_lock(&raw_notifier_lock);
>   	while (raw_busy_notifier == ro) {
> @@ -421,7 +423,7 @@ static int raw_release(struct socket *sock)
>   			raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
>   			netdev_put(ro->dev, &ro->dev_tracker);
>   		} else {
> -			raw_disable_allfilters(sock_net(sk), NULL, sk);
> +			raw_disable_allfilters(net, NULL, sk);
>   		}
>   	}
>   
> @@ -440,6 +442,7 @@ static int raw_release(struct socket *sock)
>   	release_sock(sk);
>   	rtnl_unlock();
>   
> +	sock_prot_inuse_add(net, sk->sk_prot, -1);
>   	sock_put(sk);
>   
>   	return 0;

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

[Oliver Hartkopp]

On 14.03.25 13:31, Marc Kleine-Budde wrote:
> On 14.03.2025 12:39:49, Davide Caratti wrote:
>> The third column in the output of the following command:
>>
>>   # grep CAN /proc/net/protocols
>>
>> is systematically '0': use sock_prot_inuse_add() to account for the number
>> of sockets for each protocol on top of AF_CAN family.
>>
>> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> 
> Applied to linux-can-next.
> 

Maybe too fast? E.g. J1939 is not handled.

https://lore.kernel.org/linux-can/78951192-82b1-45bc-9903-d314c94cd182@hartkopp.net/T/#m87afc41fef8ec9099344c753e32b06f302cc0e39

Best regards,
Oliver

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On 16.03.2025 13:39:16, Oliver Hartkopp wrote:
> 
> 
> On 14.03.25 13:31, Marc Kleine-Budde wrote:
> > On 14.03.2025 12:39:49, Davide Caratti wrote:
> > > The third column in the output of the following command:
> > > 
> > >   # grep CAN /proc/net/protocols
> > > 
> > > is systematically '0': use sock_prot_inuse_add() to account for the number
> > > of sockets for each protocol on top of AF_CAN family.
> > > 
> > > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > 
> > Applied to linux-can-next.
> > 
> 
> Maybe too fast? E.g. J1939 is not handled.
> 
> https://lore.kernel.org/linux-can/78951192-82b1-45bc-9903-d314c94cd182@hartkopp.net/T/#m87afc41fef8ec9099344c753e32b06f302cc0e39

Can IMHO  be added in a later patch.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Davide Caratti]

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

[...]

> ~/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; 

> 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. 

> 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...

> j1939/socket.c: sock_put(sk);
> j1939/transport.c:      sock_put(session->sk);

... except this one, that I forgot :) 

I will send a follow-up patch soon.
Thanks!
-- 
davide

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

[Oliver Hartkopp]

On 17.03.25 10:20, Marc Kleine-Budde wrote:
> On 16.03.2025 13:39:16, Oliver Hartkopp wrote:
>>
>>
>> On 14.03.25 13:31, Marc Kleine-Budde wrote:
>>> On 14.03.2025 12:39:49, Davide Caratti wrote:
>>>> The third column in the output of the following command:
>>>>
>>>>    # grep CAN /proc/net/protocols
>>>>
>>>> is systematically '0': use sock_prot_inuse_add() to account for the number
>>>> of sockets for each protocol on top of AF_CAN family.
>>>>
>>>> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
>>>
>>> Applied to linux-can-next.
>>>
>>
>> Maybe too fast? E.g. J1939 is not handled.
>>
>> https://lore.kernel.org/linux-can/78951192-82b1-45bc-9903-d314c94cd182@hartkopp.net/T/#m87afc41fef8ec9099344c753e32b06f302cc0e39
> 
> Can IMHO  be added in a later patch.

Sure?

can_create() creates all CAN sockets, right?

With this patch the sock_prot_inuse_add() increases the counter by one 
for every CAN socket.

But only RAW/BCM/ISOTP are decrementing the prot-in-use counter when 
removing the socket.

IMO this patch introduces a bug.

Best regards,
Oliver

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

[Oliver Hartkopp]

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

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

[Davide Caratti]

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

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

[Oliver Hartkopp]

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