Wolfgang Grandegger wrote:
> 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.
Yes, i thought about an inline function like
inline int no_can_skb((struct sk_buff *skb)
{
struct can_frame *cf = (struct can_frame *)skb->data
return 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);
}
so that you can have this code in each drivers xmit function:
if (no_can_skb(skb)) {
kfree_skb(skb);
return NETDEV_TX_OK;
}
Regards,
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
