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
- dev_err() is not rate limited and may cause kernel log load
But the NETDEV_TX_OK could be added into the original patch ;-)
Regards,
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
