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?
>
> Thanks,
>
> Wolfgang.
> _______________________________________________
> Socketcan-core mailing list
> [email protected]
> https://lists.berlios.de/mailman/listinfo/socketcan-core
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
