Oliver Hartkopp wrote:
> Wolfgang Grandegger wrote:
>> Hi Oliver,
>>
>> Oliver Hartkopp wrote:
>>> Wolfgang Grandegger wrote:
>>>
>>>>>> Please also check for useless "dlc > 8" checks in the
>>>>>> start_xmit function.
>>>>> I thought about that and i'm not really sure about it:
>>>>> What if anyone uses PF_PACKET with CAN netdevices, which is a possible
>>>>> use-case?
>>>> OK.
>>> Any ideas how to proceed in the start_xmit functions?
>>>
>>> We could add something like this consistently to *all* start_xmit functions:
>>>
>>> Index: drivers/net/can/sja1000/sja1000.c
>>> ===================================================================
>>> --- drivers/net/can/sja1000/sja1000.c (Revision 1095)
>>> +++ drivers/net/can/sja1000/sja1000.c (Arbeitskopie)
>>> @@ -257,6 +257,13 @@
>>> uint8_t dreg;
>>> int i;
>>>
>>> + if (WARN_ONCE(skb->len != sizeof(*cf) || cf->can_dlc > 8,
>>> + "Dropped non conform skbuf: len %d, can_dlc %d\n",
>>> + skb->len, cf->can_dlc)) {
>>> + kfree_skb(skb);
>>> + return 0;
>>> + }
>>> +
>>> netif_stop_queue(dev);
>>>
>>> fi = dlc = cf->can_dlc;
>> I'm just preparing a patch for the MSCAN touching the same topic, which
>> I have attached below. What do you think? I also wonder if we should
>> use dev_kfree_skb()?
>
> dev_kfree_skb() is mapped to consume_skb() which indicates that everything
> went ok in the receive path.
>
> IMO kfree_skb() is fine here.
>
>> - if (frame->can_dlc > 8)
>> - return -EINVAL;
>> + if (skb->len != sizeof(*frame) || frame->can_dlc > 8) {
>> + dev_err(dev->dev.parent,
>> + "Dropping non-conform paket: len %d, can_dlc %d\n",
>> + skb->len, frame->can_dlc);
>> + kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> + }
>>
>
>
> I can see several drawbacks in this implementation:
>
> - no 'unlikely()' hint for the gcc which is provided by WARN_ONCE
Well, some people care, others don't. We actually do not use it
consequently in SocketCAN, at least.
> - dev_err() is not rate limited and may cause kernel log load
Rate-limitation might be a good thing though.
> But the NETDEV_TX_OK could be added into the original patch ;-)
Should be. WARN_ONCE() is also used to trigger a TX netdev timeout and I
already got twice a *bug* report from a customer, thinking it's due to a
bug in the kernel, but he has just forgotten to connect a cable. Hope
this makes my concerns clear about using WARN_ONCE() here as well. Like
with can_get_dlc(), we could provide a can_check_frame() to ensure that
the check is done consistently.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
