[PATCH 0/2] can: bcm: random optimizations

[PATCH 0/2] can: bcm: random optimizations

[Ziyang Xuan]

Do some small optimization for can_bcm.

Ziyang Xuan (2):
  can: bcm: registration process optimization in bcm_module_init()
  can: bcm: check the result of can_send() in bcm_can_tx()

 net/can/bcm.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Ziyang Xuan]

Now, register_netdevice_notifier() and register_pernet_subsys() are both
after can_proto_register(). It can create CAN_BCM socket and process socket
once can_proto_register() successfully, so it is possible missing notifier
event or proc node creation because notifier or bcm proc directory is not
registered or created yet. Although this is a low probability scenario, it
is not impossible.

Move register_pernet_subsys() and register_netdevice_notifier() to the
front of can_proto_register(). In addition, register_pernet_subsys() and
register_netdevice_notifier() may fail, check their results are necessary.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 net/can/bcm.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index e60161bec850..e2783156bfd1 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
 
 	pr_info("can: broadcast manager protocol\n");
 
+	err = register_pernet_subsys(&canbcm_pernet_ops);
+	if (err)
+		return err;
+
+	err = register_netdevice_notifier(&canbcm_notifier);
+	if (err)
+		goto register_notifier_failed;
+
 	err = can_proto_register(&bcm_can_proto);
 	if (err < 0) {
 		printk(KERN_ERR "can: registration of bcm protocol failed\n");
-		return err;
+		goto register_proto_failed;
 	}
 
-	register_pernet_subsys(&canbcm_pernet_ops);
-	register_netdevice_notifier(&canbcm_notifier);
 	return 0;
+
+register_proto_failed:
+	unregister_netdevice_notifier(&canbcm_notifier);
+register_notifier_failed:
+	unregister_pernet_subsys(&canbcm_pernet_ops);
+	return err;
 }
 
 static void __exit bcm_module_exit(void)
-- 
2.25.1

[PATCH 2/2] can: bcm: check the result of can_send() in bcm_can_tx()

[Ziyang Xuan]

If can_send() fail, it should not update statistics in bcm_can_tx().
Add the result check for can_send() in bcm_can_tx().

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 net/can/bcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index e2783156bfd1..8f5d704a409f 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op)
 	/* send with loopback */
 	skb->dev = dev;
 	can_skb_set_owner(skb, op->sk);
-	can_send(skb, 1);
+	if (can_send(skb, 1))
+		goto out;
 
 	/* update statistics */
 	op->currframe++;
-- 
2.25.1

Re: [PATCH 2/2] can: bcm: check the result of can_send() in bcm_can_tx()

[Oliver Hartkopp]

Sorry, but NACK.

The curr_frame counter handles the sequence counter of multiplex messages.

Even when this single send attempt failed the curr_frame counter has to 
continue.

For that reason the comment about statistics *before* the curr_frame++ 
might be misleading.

A potential improvement could be:

	if (!(can_send(skb, 1)))
		op->frames_abs++;

	op->currframe++;

But as op->frames_abs is a functional unused(!) value for tx ops and 
only displayed via procfs I would NOT tag such improvement as a 'fix' 
which might then be queued up for stable.

This could be something for the can-next tree ...

Best regards,
Oliver


On 08.09.22 05:04, Ziyang Xuan wrote:
> If can_send() fail, it should not update statistics in bcm_can_tx().
> Add the result check for can_send() in bcm_can_tx().
> 
> Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>   net/can/bcm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index e2783156bfd1..8f5d704a409f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op)
>   	/* send with loopback */
>   	skb->dev = dev;
>   	can_skb_set_owner(skb, op->sk);
> -	can_send(skb, 1);
> +	if (can_send(skb, 1))
> +		goto out;
>   
>   	/* update statistics */
>   	op->currframe++;

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Oliver Hartkopp]

On 08.09.22 05:04, Ziyang Xuan wrote:
> Now, register_netdevice_notifier() and register_pernet_subsys() are both
> after can_proto_register(). It can create CAN_BCM socket and process socket
> once can_proto_register() successfully, so it is possible missing notifier
> event or proc node creation because notifier or bcm proc directory is not
> registered or created yet. Although this is a low probability scenario, it
> is not impossible.
> 
> Move register_pernet_subsys() and register_netdevice_notifier() to the
> front of can_proto_register(). In addition, register_pernet_subsys() and
> register_netdevice_notifier() may fail, check their results are necessary.
> 
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>   net/can/bcm.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index e60161bec850..e2783156bfd1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
>   
>   	pr_info("can: broadcast manager protocol\n");
>   
> +	err = register_pernet_subsys(&canbcm_pernet_ops);
> +	if (err)
> +		return err;

Analogue to your patch for the CAN_RAW socket here (which has been 
applied to can-next right now) ...

https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u

... I'm not sure whether this is the right sequence to acquire the 
different resources here.

E.g. in ipsec_pfkey_init() in af_key.c

https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887

proto_register() is executed before register_pernet_subsys()

Which seems to be more natural to me.

Best regards,
Oliver

> +
> +	err = register_netdevice_notifier(&canbcm_notifier);
> +	if (err)
> +		goto register_notifier_failed;
> +
>   	err = can_proto_register(&bcm_can_proto);
>   	if (err < 0) {
>   		printk(KERN_ERR "can: registration of bcm protocol failed\n");
> -		return err;
> +		goto register_proto_failed;
>   	}
>   
> -	register_pernet_subsys(&canbcm_pernet_ops);
> -	register_netdevice_notifier(&canbcm_notifier);
>   	return 0;
> +
> +register_proto_failed:
> +	unregister_netdevice_notifier(&canbcm_notifier);
> +register_notifier_failed:
> +	unregister_pernet_subsys(&canbcm_pernet_ops);
> +	return err;
>   }
>   
>   static void __exit bcm_module_exit(void)

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Oliver Hartkopp]

Just another reference which make it clear that the reordering of 
function calls in your patch is likely not correct:

https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734

static int __init packet_init(void)
{
         int rc;

         rc = proto_register(&packet_proto, 0);
         if (rc)
                 goto out;
         rc = sock_register(&packet_family_ops);
         if (rc)
                 goto out_proto;
         rc = register_pernet_subsys(&packet_net_ops);
         if (rc)
                 goto out_sock;
         rc = register_netdevice_notifier(&packet_netdev_notifier);
         if (rc)
                 goto out_pernet;

         return 0;

out_pernet:
         unregister_pernet_subsys(&packet_net_ops);
out_sock:
         sock_unregister(PF_PACKET);
out_proto:
         proto_unregister(&packet_proto);
out:
         return rc;
}



On 08.09.22 09:10, Oliver Hartkopp wrote:
> 
> 
> On 08.09.22 05:04, Ziyang Xuan wrote:
>> Now, register_netdevice_notifier() and register_pernet_subsys() are both
>> after can_proto_register(). It can create CAN_BCM socket and process 
>> socket
>> once can_proto_register() successfully, so it is possible missing 
>> notifier
>> event or proc node creation because notifier or bcm proc directory is not
>> registered or created yet. Although this is a low probability 
>> scenario, it
>> is not impossible.
>>
>> Move register_pernet_subsys() and register_netdevice_notifier() to the
>> front of can_proto_register(). In addition, register_pernet_subsys() and
>> register_netdevice_notifier() may fail, check their results are 
>> necessary.
>>
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>> ---
>>   net/can/bcm.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index e60161bec850..e2783156bfd1 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
>>       pr_info("can: broadcast manager protocol\n");
>> +    err = register_pernet_subsys(&canbcm_pernet_ops);
>> +    if (err)
>> +        return err;
> 
> Analogue to your patch for the CAN_RAW socket here (which has been 
> applied to can-next right now) ...
> 
> https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u 
> 
> 
> ... I'm not sure whether this is the right sequence to acquire the 
> different resources here.
> 
> E.g. in ipsec_pfkey_init() in af_key.c
> 
> https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887
> 
> proto_register() is executed before register_pernet_subsys()
> 
> Which seems to be more natural to me.
> 
> Best regards,
> Oliver
> 
>> +
>> +    err = register_netdevice_notifier(&canbcm_notifier);
>> +    if (err)
>> +        goto register_notifier_failed;
>> +
>>       err = can_proto_register(&bcm_can_proto);
>>       if (err < 0) {
>>           printk(KERN_ERR "can: registration of bcm protocol failed\n");
>> -        return err;
>> +        goto register_proto_failed;
>>       }
>> -    register_pernet_subsys(&canbcm_pernet_ops);
>> -    register_netdevice_notifier(&canbcm_notifier);
>>       return 0;
>> +
>> +register_proto_failed:
>> +    unregister_netdevice_notifier(&canbcm_notifier);
>> +register_notifier_failed:
>> +    unregister_pernet_subsys(&canbcm_pernet_ops);
>> +    return err;
>>   }
>>   static void __exit bcm_module_exit(void)

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Ziyang Xuan (William)]

> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
> 
> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
> 
> static int __init packet_init(void)
> {
>         int rc;
> 
>         rc = proto_register(&packet_proto, 0);
>         if (rc)
>                 goto out;
>         rc = sock_register(&packet_family_ops);
>         if (rc)
>                 goto out_proto;
>         rc = register_pernet_subsys(&packet_net_ops);
>         if (rc)
>                 goto out_sock;
>         rc = register_netdevice_notifier(&packet_netdev_notifier);
>         if (rc)
>                 goto out_pernet;
> 
>         return 0;
> 
> out_pernet:
>         unregister_pernet_subsys(&packet_net_ops);
> out_sock:
>         sock_unregister(PF_PACKET);
> out_proto:
>         proto_unregister(&packet_proto);
> out:
>         return rc;
> }
> 

I had a simple test with can_raw. kernel modification as following:

--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -118,6 +118,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
        const struct can_proto *cp;
        int err = 0;

+       printk("%s: protocol: %d\n", __func__, protocol);
+
        sock->state = SS_UNCONNECTED;

        if (protocol < 0 || protocol >= CAN_NPROTO)
diff --git a/net/can/raw.c b/net/can/raw.c
index 5dca1e9e44cf..6052fd0cc7b2 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -943,6 +943,9 @@ static __init int raw_module_init(void)
        pr_info("can: raw protocol\n");

        err = can_proto_register(&raw_can_proto);
+       printk("%s: can_proto_register done\n", __func__);
+       msleep(5000); // 5s
+       printk("%s: to register_netdevice_notifier\n", __func__);
        if (err < 0)
                pr_err("can: registration of raw protocol failed\n");
        else

I added 5 seconds delay after can_proto_register() and some debugs.
Testcase codes just try to create a CAN_RAW socket in user space as following:

int main(int argc, char **argv)
{
        int s;

        s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
        if (s < 0) {
                perror("socket");
                return 0;
        }
        close(s);
        return 0;
}

Execute 'modprobe can_raw' and the testcase we can get message as following:

[  109.312767] can: raw protocol
[  109.312772] raw_module_init: can_proto_register done
[  111.296178] can_create: protocol: 1
[  114.809141] raw_module_init: to register_netdevice_notifier

It proved that it can create CAN_RAW socket and process socket once can_proto_register() successfully.
CAN_BCM is the same.

In the vast majority of cases, creating protocol socket and operating it are after protocol module initialization.
The scenario that I pointed in my patch is a low probability.

af_packet.c and af_key.c do like that doesn't mean it's very correct. I think so.

Thank you for your prompt reply.

> 
> 
> On 08.09.22 09:10, Oliver Hartkopp wrote:
>>
>>
>> On 08.09.22 05:04, Ziyang Xuan wrote:
>>> Now, register_netdevice_notifier() and register_pernet_subsys() are both
>>> after can_proto_register(). It can create CAN_BCM socket and process socket
>>> once can_proto_register() successfully, so it is possible missing notifier
>>> event or proc node creation because notifier or bcm proc directory is not
>>> registered or created yet. Although this is a low probability scenario, it
>>> is not impossible.
>>>
>>> Move register_pernet_subsys() and register_netdevice_notifier() to the
>>> front of can_proto_register(). In addition, register_pernet_subsys() and
>>> register_netdevice_notifier() may fail, check their results are necessary.
>>>
>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>>> ---
>>>   net/can/bcm.c | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>>> index e60161bec850..e2783156bfd1 100644
>>> --- a/net/can/bcm.c
>>> +++ b/net/can/bcm.c
>>> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
>>>       pr_info("can: broadcast manager protocol\n");
>>> +    err = register_pernet_subsys(&canbcm_pernet_ops);
>>> +    if (err)
>>> +        return err;
>>
>> Analogue to your patch for the CAN_RAW socket here (which has been applied to can-next right now) ...
>>
>> https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u
>>
>> ... I'm not sure whether this is the right sequence to acquire the different resources here.
>>
>> E.g. in ipsec_pfkey_init() in af_key.c
>>
>> https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887
>>
>> proto_register() is executed before register_pernet_subsys()
>>
>> Which seems to be more natural to me.
>>
>> Best regards,
>> Oliver
>>
>>> +
>>> +    err = register_netdevice_notifier(&canbcm_notifier);
>>> +    if (err)
>>> +        goto register_notifier_failed;
>>> +
>>>       err = can_proto_register(&bcm_can_proto);
>>>       if (err < 0) {
>>>           printk(KERN_ERR "can: registration of bcm protocol failed\n");
>>> -        return err;
>>> +        goto register_proto_failed;
>>>       }
>>> -    register_pernet_subsys(&canbcm_pernet_ops);
>>> -    register_netdevice_notifier(&canbcm_notifier);
>>>       return 0;
>>> +
>>> +register_proto_failed:
>>> +    unregister_netdevice_notifier(&canbcm_notifier);
>>> +register_notifier_failed:
>>> +    unregister_pernet_subsys(&canbcm_pernet_ops);
>>> +    return err;
>>>   }
>>>   static void __exit bcm_module_exit(void)
> .

Re: [PATCH 2/2] can: bcm: check the result of can_send() in bcm_can_tx()

[Ziyang Xuan (William)]

> Sorry, but NACK.
> 
> The curr_frame counter handles the sequence counter of multiplex messages.
> 
> Even when this single send attempt failed the curr_frame counter has to continue.
> 
> For that reason the comment about statistics *before* the curr_frame++ might be misleading.
> 
> A potential improvement could be:
> 
>     if (!(can_send(skb, 1)))
>         op->frames_abs++;
> 
>     op->currframe++;
> 
> But as op->frames_abs is a functional unused(!) value for tx ops and only displayed via procfs I would NOT tag such improvement as a 'fix' which might then be queued up for stable.
> 
I will modify and remove 'Fixes' tag in v2.

Thank you for your review.

> This could be something for the can-next tree ...
> 
> Best regards,
> Oliver
> 
> 
> On 08.09.22 05:04, Ziyang Xuan wrote:
>> If can_send() fail, it should not update statistics in bcm_can_tx().
>> Add the result check for can_send() in bcm_can_tx().
>>
>> Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>> ---
>>   net/can/bcm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index e2783156bfd1..8f5d704a409f 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op)
>>       /* send with loopback */
>>       skb->dev = dev;
>>       can_skb_set_owner(skb, op->sk);
>> -    can_send(skb, 1);
>> +    if (can_send(skb, 1))
>> +        goto out;
>>         /* update statistics */
>>       op->currframe++;
> .

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Oliver Hartkopp]

On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>
>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>
>> static int __init packet_init(void)
>> {
>>          int rc;
>>
>>          rc = proto_register(&packet_proto, 0);
>>          if (rc)
>>                  goto out;
>>          rc = sock_register(&packet_family_ops);
>>          if (rc)
>>                  goto out_proto;
>>          rc = register_pernet_subsys(&packet_net_ops);
>>          if (rc)
>>                  goto out_sock;
>>          rc = register_netdevice_notifier(&packet_netdev_notifier);
>>          if (rc)
>>                  goto out_pernet;
>>
>>          return 0;
>>
>> out_pernet:
>>          unregister_pernet_subsys(&packet_net_ops);
>> out_sock:
>>          sock_unregister(PF_PACKET);
>> out_proto:
>>          proto_unregister(&packet_proto);
>> out:
>>          return rc;
>> }
>>
> 
> I had a simple test with can_raw. kernel modification as following:
> 
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -118,6 +118,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>          const struct can_proto *cp;
>          int err = 0;
> 
> +       printk("%s: protocol: %d\n", __func__, protocol);
> +
>          sock->state = SS_UNCONNECTED;
> 
>          if (protocol < 0 || protocol >= CAN_NPROTO)
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 5dca1e9e44cf..6052fd0cc7b2 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -943,6 +943,9 @@ static __init int raw_module_init(void)
>          pr_info("can: raw protocol\n");
> 
>          err = can_proto_register(&raw_can_proto);
> +       printk("%s: can_proto_register done\n", __func__);
> +       msleep(5000); // 5s
> +       printk("%s: to register_netdevice_notifier\n", __func__);
>          if (err < 0)
>                  pr_err("can: registration of raw protocol failed\n");
>          else
> 
> I added 5 seconds delay after can_proto_register() and some debugs.
> Testcase codes just try to create a CAN_RAW socket in user space as following:
> 
> int main(int argc, char **argv)
> {
>          int s;
> 
>          s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
>          if (s < 0) {
>                  perror("socket");
>                  return 0;
>          }
>          close(s);
>          return 0;
> }
> 
> Execute 'modprobe can_raw' and the testcase we can get message as following:
> 
> [  109.312767] can: raw protocol
> [  109.312772] raw_module_init: can_proto_register done
> [  111.296178] can_create: protocol: 1
> [  114.809141] raw_module_init: to register_netdevice_notifier
> 
> It proved that it can create CAN_RAW socket and process socket once can_proto_register() successfully.
> CAN_BCM is the same.

Well, opening a CAN_RAW socket is not a proof that you can delay 
register_netdevice_notifier() that much.

After creating the socket you need to set the netdevice and can add some 
CAN filters and execute bind() on that socket.

And these filters need to be removed be the netdev notifier when someone 
plugs out the USB CAN adapter.

> In the vast majority of cases, creating protocol socket and operating it are after protocol module initialization.
> The scenario that I pointed in my patch is a low probability.
> 
> af_packet.c and af_key.c do like that doesn't mean it's very correct. I think so.

I'm not sure either and this is why I'm asking.

Maybe having the notifier enabled first does not have a negative effect 
when removing the USB CAN interface when there is CAN_RAW protocol has 
been registered.

But if so, the PF_PACKET code should be revisited too.

Best regards,
Oliver

> 
> Thank you for your prompt reply.
> 
>>
>>
>> On 08.09.22 09:10, Oliver Hartkopp wrote:
>>>
>>>
>>> On 08.09.22 05:04, Ziyang Xuan wrote:
>>>> Now, register_netdevice_notifier() and register_pernet_subsys() are both
>>>> after can_proto_register(). It can create CAN_BCM socket and process socket
>>>> once can_proto_register() successfully, so it is possible missing notifier
>>>> event or proc node creation because notifier or bcm proc directory is not
>>>> registered or created yet. Although this is a low probability scenario, it
>>>> is not impossible.
>>>>
>>>> Move register_pernet_subsys() and register_netdevice_notifier() to the
>>>> front of can_proto_register(). In addition, register_pernet_subsys() and
>>>> register_netdevice_notifier() may fail, check their results are necessary.
>>>>
>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>>>> ---
>>>>    net/can/bcm.c | 18 +++++++++++++++---
>>>>    1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>>>> index e60161bec850..e2783156bfd1 100644
>>>> --- a/net/can/bcm.c
>>>> +++ b/net/can/bcm.c
>>>> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
>>>>        pr_info("can: broadcast manager protocol\n");
>>>> +    err = register_pernet_subsys(&canbcm_pernet_ops);
>>>> +    if (err)
>>>> +        return err;
>>>
>>> Analogue to your patch for the CAN_RAW socket here (which has been applied to can-next right now) ...
>>>
>>> https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u
>>>
>>> ... I'm not sure whether this is the right sequence to acquire the different resources here.
>>>
>>> E.g. in ipsec_pfkey_init() in af_key.c
>>>
>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887
>>>
>>> proto_register() is executed before register_pernet_subsys()
>>>
>>> Which seems to be more natural to me.
>>>
>>> Best regards,
>>> Oliver
>>>
>>>> +
>>>> +    err = register_netdevice_notifier(&canbcm_notifier);
>>>> +    if (err)
>>>> +        goto register_notifier_failed;
>>>> +
>>>>        err = can_proto_register(&bcm_can_proto);
>>>>        if (err < 0) {
>>>>            printk(KERN_ERR "can: registration of bcm protocol failed\n");
>>>> -        return err;
>>>> +        goto register_proto_failed;
>>>>        }
>>>> -    register_pernet_subsys(&canbcm_pernet_ops);
>>>> -    register_netdevice_notifier(&canbcm_notifier);
>>>>        return 0;
>>>> +
>>>> +register_proto_failed:
>>>> +    unregister_netdevice_notifier(&canbcm_notifier);
>>>> +register_notifier_failed:
>>>> +    unregister_pernet_subsys(&canbcm_pernet_ops);
>>>> +    return err;
>>>>    }
>>>>    static void __exit bcm_module_exit(void)
>> .

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Ziyang Xuan (William)]

> 
> 
> On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>>
>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>>
>>> static int __init packet_init(void)
>>> {
>>>          int rc;
>>>
>>>          rc = proto_register(&packet_proto, 0);
>>>          if (rc)
>>>                  goto out;
>>>          rc = sock_register(&packet_family_ops);
>>>          if (rc)
>>>                  goto out_proto;
>>>          rc = register_pernet_subsys(&packet_net_ops);
>>>          if (rc)
>>>                  goto out_sock;
>>>          rc = register_netdevice_notifier(&packet_netdev_notifier);
>>>          if (rc)
>>>                  goto out_pernet;
>>>
>>>          return 0;
>>>
>>> out_pernet:
>>>          unregister_pernet_subsys(&packet_net_ops);
>>> out_sock:
>>>          sock_unregister(PF_PACKET);
>>> out_proto:
>>>          proto_unregister(&packet_proto);
>>> out:
>>>          return rc;
>>> }
>>>
>>
>> I had a simple test with can_raw. kernel modification as following:
>>
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -118,6 +118,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>>          const struct can_proto *cp;
>>          int err = 0;
>>
>> +       printk("%s: protocol: %d\n", __func__, protocol);
>> +
>>          sock->state = SS_UNCONNECTED;
>>
>>          if (protocol < 0 || protocol >= CAN_NPROTO)
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index 5dca1e9e44cf..6052fd0cc7b2 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -943,6 +943,9 @@ static __init int raw_module_init(void)
>>          pr_info("can: raw protocol\n");
>>
>>          err = can_proto_register(&raw_can_proto);
>> +       printk("%s: can_proto_register done\n", __func__);
>> +       msleep(5000); // 5s
>> +       printk("%s: to register_netdevice_notifier\n", __func__);
>>          if (err < 0)
>>                  pr_err("can: registration of raw protocol failed\n");
>>          else
>>
>> I added 5 seconds delay after can_proto_register() and some debugs.
>> Testcase codes just try to create a CAN_RAW socket in user space as following:
>>
>> int main(int argc, char **argv)
>> {
>>          int s;
>>
>>          s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
>>          if (s < 0) {
>>                  perror("socket");
>>                  return 0;
>>          }
>>          close(s);
>>          return 0;
>> }
>>
>> Execute 'modprobe can_raw' and the testcase we can get message as following:
>>
>> [  109.312767] can: raw protocol
>> [  109.312772] raw_module_init: can_proto_register done
>> [  111.296178] can_create: protocol: 1
>> [  114.809141] raw_module_init: to register_netdevice_notifier
>>
>> It proved that it can create CAN_RAW socket and process socket once can_proto_register() successfully.
>> CAN_BCM is the same.
> 
> Well, opening a CAN_RAW socket is not a proof that you can delay register_netdevice_notifier() that much.
> 
> After creating the socket you need to set the netdevice and can add some CAN filters and execute bind() on that socket.

Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().

If the condition which I pointed happens, I think my solution can solve.

> 
> And these filters need to be removed be the netdev notifier when someone plugs out the USB CAN adapter.
> 
>> In the vast majority of cases, creating protocol socket and operating it are after protocol module initialization.
>> The scenario that I pointed in my patch is a low probability.
>>
>> af_packet.c and af_key.c do like that doesn't mean it's very correct. I think so.
> 
> I'm not sure either and this is why I'm asking.
> 
> Maybe having the notifier enabled first does not have a negative effect when removing the USB CAN interface when there is CAN_RAW protocol has been registered.
> 
> But if so, the PF_PACKET code should be revisited too
It is a low probability scenario. Maybe not everyone agrees that it is worth it. But I will try to speak my voice.

Thank you.

> 
> Best regards,
> Oliver
> 
>>
>> Thank you for your prompt reply.
>>
>>>
>>>
>>> On 08.09.22 09:10, Oliver Hartkopp wrote:
>>>>
>>>>
>>>> On 08.09.22 05:04, Ziyang Xuan wrote:
>>>>> Now, register_netdevice_notifier() and register_pernet_subsys() are both
>>>>> after can_proto_register(). It can create CAN_BCM socket and process socket
>>>>> once can_proto_register() successfully, so it is possible missing notifier
>>>>> event or proc node creation because notifier or bcm proc directory is not
>>>>> registered or created yet. Although this is a low probability scenario, it
>>>>> is not impossible.
>>>>>
>>>>> Move register_pernet_subsys() and register_netdevice_notifier() to the
>>>>> front of can_proto_register(). In addition, register_pernet_subsys() and
>>>>> register_netdevice_notifier() may fail, check their results are necessary.
>>>>>
>>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>>>>> ---
>>>>>    net/can/bcm.c | 18 +++++++++++++++---
>>>>>    1 file changed, 15 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>>>>> index e60161bec850..e2783156bfd1 100644
>>>>> --- a/net/can/bcm.c
>>>>> +++ b/net/can/bcm.c
>>>>> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void)
>>>>>        pr_info("can: broadcast manager protocol\n");
>>>>> +    err = register_pernet_subsys(&canbcm_pernet_ops);
>>>>> +    if (err)
>>>>> +        return err;
>>>>
>>>> Analogue to your patch for the CAN_RAW socket here (which has been applied to can-next right now) ...
>>>>
>>>> https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u
>>>>
>>>> ... I'm not sure whether this is the right sequence to acquire the different resources here.
>>>>
>>>> E.g. in ipsec_pfkey_init() in af_key.c
>>>>
>>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887
>>>>
>>>> proto_register() is executed before register_pernet_subsys()
>>>>
>>>> Which seems to be more natural to me.
>>>>
>>>> Best regards,
>>>> Oliver
>>>>
>>>>> +
>>>>> +    err = register_netdevice_notifier(&canbcm_notifier);
>>>>> +    if (err)
>>>>> +        goto register_notifier_failed;
>>>>> +
>>>>>        err = can_proto_register(&bcm_can_proto);
>>>>>        if (err < 0) {
>>>>>            printk(KERN_ERR "can: registration of bcm protocol failed\n");
>>>>> -        return err;
>>>>> +        goto register_proto_failed;
>>>>>        }
>>>>> -    register_pernet_subsys(&canbcm_pernet_ops);
>>>>> -    register_netdevice_notifier(&canbcm_notifier);
>>>>>        return 0;
>>>>> +
>>>>> +register_proto_failed:
>>>>> +    unregister_netdevice_notifier(&canbcm_notifier);
>>>>> +register_notifier_failed:
>>>>> +    unregister_pernet_subsys(&canbcm_pernet_ops);
>>>>> +    return err;
>>>>>    }
>>>>>    static void __exit bcm_module_exit(void)
>>> .
> .

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Oliver Hartkopp]

On 09.09.22 05:58, Ziyang Xuan (William) wrote:
>>
>>
>> On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>>>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>>>
>>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>>>
>>>> static int __init packet_init(void)
>>>> {
>>>>           int rc;
>>>>
>>>>           rc = proto_register(&packet_proto, 0);
>>>>           if (rc)
>>>>                   goto out;
>>>>           rc = sock_register(&packet_family_ops);
>>>>           if (rc)
>>>>                   goto out_proto;
>>>>           rc = register_pernet_subsys(&packet_net_ops);
>>>>           if (rc)
>>>>                   goto out_sock;
>>>>           rc = register_netdevice_notifier(&packet_netdev_notifier);
>>>>           if (rc)
>>>>                   goto out_pernet;
>>>>
>>>>           return 0;
>>>>
>>>> out_pernet:
>>>>           unregister_pernet_subsys(&packet_net_ops);
>>>> out_sock:
>>>>           sock_unregister(PF_PACKET);
>>>> out_proto:
>>>>           proto_unregister(&packet_proto);
>>>> out:
>>>>           return rc;
>>>> }
>>>>

> Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
> But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
> or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
> 
> If the condition which I pointed happens, I think my solution can solve.
> 

No, I don't think so.

We need to maintain the exact order which is depicted in the af_packet.c 
code from above as the notifier call references the sock pointer.

Regards,
Oliver

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Marc Kleine-Budde]

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

On 09.09.2022 17:04:06, Oliver Hartkopp wrote:
> 
> 
> On 09.09.22 05:58, Ziyang Xuan (William) wrote:
> > > 
> > > 
> > > On 9/8/22 13:14, Ziyang Xuan (William) wrote:
> > > > > Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
> > > > > 
> > > > > https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
> > > > > 
> > > > > static int __init packet_init(void)
> > > > > {
> > > > >           int rc;
> > > > > 
> > > > >           rc = proto_register(&packet_proto, 0);
> > > > >           if (rc)
> > > > >                   goto out;
> > > > >           rc = sock_register(&packet_family_ops);
> > > > >           if (rc)
> > > > >                   goto out_proto;
> > > > >           rc = register_pernet_subsys(&packet_net_ops);
> > > > >           if (rc)
> > > > >                   goto out_sock;
> > > > >           rc = register_netdevice_notifier(&packet_netdev_notifier);
> > > > >           if (rc)
> > > > >                   goto out_pernet;
> > > > > 
> > > > >           return 0;
> > > > > 
> > > > > out_pernet:
> > > > >           unregister_pernet_subsys(&packet_net_ops);
> > > > > out_sock:
> > > > >           sock_unregister(PF_PACKET);
> > > > > out_proto:
> > > > >           proto_unregister(&packet_proto);
> > > > > out:
> > > > >           return rc;
> > > > > }
> > > > > 
> 
> > Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
> > But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
> > or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
> > 
> > If the condition which I pointed happens, I think my solution can solve.
> > 
> 
> No, I don't think so.
> 
> We need to maintain the exact order which is depicted in the af_packet.c
> code from above as the notifier call references the sock pointer.

The notifier calls bcm_notifier() first, which will loop over the
bcm_notifier_list. The list is empty if there are no sockets open, yet.
So from my point of view this change looks fine.

IMHO it's better to make a series where all these notifiers are moved in
front of the respective socket proto_register().

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 2/2] can: bcm: check the result of can_send() in bcm_can_tx()

[Marc Kleine-Budde]

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

On 08.09.2022 08:47:57, Oliver Hartkopp wrote:
> Sorry, but NACK.
> 
> The curr_frame counter handles the sequence counter of multiplex messages.
> 
> Even when this single send attempt failed the curr_frame counter has to
> continue.
> 
> For that reason the comment about statistics *before* the curr_frame++ might
> be misleading.
> 
> A potential improvement could be:
> 
> 	if (!(can_send(skb, 1)))

Nitpick:
In the kernel we usually assign the return value to a variable first,
and evaluate this variable in the if ().

> 		op->frames_abs++;
> 
> 	op->currframe++;

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Oliver Hartkopp]

On 12.09.22 14:00, Marc Kleine-Budde wrote:
> On 09.09.2022 17:04:06, Oliver Hartkopp wrote:
>>
>>
>> On 09.09.22 05:58, Ziyang Xuan (William) wrote:
>>>>
>>>>
>>>> On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>>>>>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>>>>>
>>>>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>>>>>
>>>>>> static int __init packet_init(void)
>>>>>> {
>>>>>>            int rc;
>>>>>>
>>>>>>            rc = proto_register(&packet_proto, 0);
>>>>>>            if (rc)
>>>>>>                    goto out;
>>>>>>            rc = sock_register(&packet_family_ops);
>>>>>>            if (rc)
>>>>>>                    goto out_proto;
>>>>>>            rc = register_pernet_subsys(&packet_net_ops);
>>>>>>            if (rc)
>>>>>>                    goto out_sock;
>>>>>>            rc = register_netdevice_notifier(&packet_netdev_notifier);
>>>>>>            if (rc)
>>>>>>                    goto out_pernet;
>>>>>>
>>>>>>            return 0;
>>>>>>
>>>>>> out_pernet:
>>>>>>            unregister_pernet_subsys(&packet_net_ops);
>>>>>> out_sock:
>>>>>>            sock_unregister(PF_PACKET);
>>>>>> out_proto:
>>>>>>            proto_unregister(&packet_proto);
>>>>>> out:
>>>>>>            return rc;
>>>>>> }
>>>>>>
>>
>>> Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
>>> But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
>>> or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
>>>
>>> If the condition which I pointed happens, I think my solution can solve.
>>>
>>
>> No, I don't think so.
>>
>> We need to maintain the exact order which is depicted in the af_packet.c
>> code from above as the notifier call references the sock pointer.
> 
> The notifier calls bcm_notifier() first, which will loop over the
> bcm_notifier_list. The list is empty if there are no sockets open, yet.
> So from my point of view this change looks fine.
> 
> IMHO it's better to make a series where all these notifiers are moved in
> front of the respective socket proto_register().

Notifiers and/or pernet_subsys ?

But yes, that would be better to have a clean consistent sequence in all 
these cases.

Would this affect af_packet.c then too?

Regards,
Oliver

Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init()

[Ziyang Xuan (William)]

> 
> 
> On 12.09.22 14:00, Marc Kleine-Budde wrote:
>> On 09.09.2022 17:04:06, Oliver Hartkopp wrote:
>>>
>>>
>>> On 09.09.22 05:58, Ziyang Xuan (William) wrote:
>>>>>
>>>>>
>>>>> On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>>>>>>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>>>>>>
>>>>>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>>>>>>
>>>>>>> static int __init packet_init(void)
>>>>>>> {
>>>>>>>            int rc;
>>>>>>>
>>>>>>>            rc = proto_register(&packet_proto, 0);
>>>>>>>            if (rc)
>>>>>>>                    goto out;
>>>>>>>            rc = sock_register(&packet_family_ops);
>>>>>>>            if (rc)
>>>>>>>                    goto out_proto;
>>>>>>>            rc = register_pernet_subsys(&packet_net_ops);
>>>>>>>            if (rc)
>>>>>>>                    goto out_sock;
>>>>>>>            rc = register_netdevice_notifier(&packet_netdev_notifier);
>>>>>>>            if (rc)
>>>>>>>                    goto out_pernet;
>>>>>>>
>>>>>>>            return 0;
>>>>>>>
>>>>>>> out_pernet:
>>>>>>>            unregister_pernet_subsys(&packet_net_ops);
>>>>>>> out_sock:
>>>>>>>            sock_unregister(PF_PACKET);
>>>>>>> out_proto:
>>>>>>>            proto_unregister(&packet_proto);
>>>>>>> out:
>>>>>>>            return rc;
>>>>>>> }
>>>>>>>
>>>
>>>> Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
>>>> But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
>>>> or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
>>>>
>>>> If the condition which I pointed happens, I think my solution can solve.
>>>>
>>>
>>> No, I don't think so.
>>>
>>> We need to maintain the exact order which is depicted in the af_packet.c
>>> code from above as the notifier call references the sock pointer.
>>
>> The notifier calls bcm_notifier() first, which will loop over the
>> bcm_notifier_list. The list is empty if there are no sockets open, yet.
>> So from my point of view this change looks fine.
>>
>> IMHO it's better to make a series where all these notifiers are moved in
>> front of the respective socket proto_register().
> 
> Notifiers and/or pernet_subsys ?
> 
> But yes, that would be better to have a clean consistent sequence in all these cases.
> 
> Would this affect af_packet.c then too?
Yes.

When we create a sock by packet_create() after proto_register() and sock_register().
It will use net->packet.sklist_lock and net->packet.sklist directly in packet_create().
net->packet.sklist_lock and net->packet.sklist are initialized in packet_net_init().

The code snippet is as follows:

static int packet_create(struct net *net, struct socket *sock, int protocol,
			 int kern)
{
	...
	mutex_lock(&net->packet.sklist_lock);
	sk_add_node_tail_rcu(sk, &net->packet.sklist);
	mutex_unlock(&net->packet.sklist_lock);
	...
}


static int __net_init packet_net_init(struct net *net)
{
	mutex_init(&net->packet.sklist_lock);
	INIT_HLIST_HEAD(&net->packet.sklist);
	...
}

So, if the sock is created firstly, we will get illegal access bug.

> 
> Regards,
> Oliver
> 
> .