Kurt Van Dijck wrote:
> On Sun, Dec 13, 2009 at 09:17:37PM +0100, Wolfgang Grandegger wrote:
>> 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)) {
> if (unlikely(no_can_skb(skb))) {
>>> kfree_skb(skb);
>>> return NETDEV_TX_OK;
>>> }
>>>
> This approach looks good. It could lead to uniform behaviour, which is a
> benefit for all.
>> 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.
> I share a bit the concern, but if I understand well, this is to avoid
> PF_PACKET sockets to break CAN rules. When regular PF_CAN is used, the
> user will be informed via can_send in af_can.c, isn't it?
Yes, that's also my understanding:
http://lxr.linux.no/#linux+v2.6.32/net/can/af_can.c#L218
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
