Oliver Hartkopp wrote:
> 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;
> }
>
I still believe that the message and the backtrace does not help the
user to understand that he has done something wrong. Nevertheless, if
there is nobody else sharing my concerns, please go ahead preparing the
patch as you suggested above. The future will show if they are reasonable.
Thanks,
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
