Messages in this thread | | | Date | Thu, 12 May 2022 08:23:26 +0200 | Subject | Re: [PATCH 1/1] can: skb: add and set local_origin flag | From | Oliver Hartkopp <> |
| |
Hi Marc,
On 11.05.22 16:57, Oliver Hartkopp wrote: > > > On 5/11/22 16:54, Marc Kleine-Budde wrote: >> On 11.05.2022 16:50:06, Oliver Hartkopp wrote: >>> >>> >>> On 5/11/22 16:36, Marc Kleine-Budde wrote: >>>> On 11.05.2022 15:24:21, Marc Kleine-Budde wrote: >>>>> On 11.05.2022 14:38:32, Oliver Hartkopp wrote: >>>>>> IMO this patch does not work as intended. >>>>>> >>>>>> You probably need to revisit every place where can_skb_reserve() >>>>>> is used, >>>>>> e.g. in raw_sendmsg(). >>>>> >>>>> And the loopback for devices that don't support IFF_ECHO: >>>>> >>>>> | https://elixir.bootlin.com/linux/latest/source/net/can/af_can.c#L257 >>>> >>>> BTW: There is a bug with interfaces that don't support IFF_ECHO. >>>> >>>> Assume an invalid CAN frame is passed to can_send() on an interface >>>> that >>>> doesn't support IFF_ECHO. The above mentioned code does happily >>>> generate >>>> an echo frame and it's send, even if the driver drops it, due to >>>> can_dropped_invalid_skb(dev, skb). >>>> >>>> The echoed back CAN frame is treated in raw_rcv() as if the headroom >>>> is valid: I double checked that code and when I didn't miss anything all the callers of can_send() (e.g. raw_sendmsg()) are creating valid skbs.
https://elixir.bootlin.com/linux/v5.17.6/A/ident/can_send
>>>> >>>> | https://elixir.bootlin.com/linux/v5.17.6/source/net/can/raw.c#L138 >>>> >>>> But as far as I can see the can_skb_headroom_valid() check never has >>>> been done. What about this patch? >>>> >>>> index 1fb49d51b25d..fda4807ad165 100644 >>>> --- a/net/can/af_can.c >>>> +++ b/net/can/af_can.c >>>> @@ -255,6 +255,9 @@ int can_send(struct sk_buff *skb, int loop) >>>> */ >>>> if (!(skb->dev->flags & IFF_ECHO)) { >>>> + if (can_dropped_invalid_skb(dev, skb)) >>>> + return -EINVAL; >>>> +
That would make this change unnecessary, right?
IIRC the reason for can_dropped_invalid_skb() is to prove valid skbs for CAN interface drivers when CAN frame skbs are created e.g. with PF_PACKET sockets.
Best, Oliver
>>> >>> Good point! >>> >>> But please check the rest of the code. >>> You need 'goto inval_skb;' instead of the return ;-) >> >> Why? To free the skb? That's what can_dropped_invalid_skb() does, too: >> >> | >> https://elixir.bootlin.com/linux/v5.17.6/source/include/linux/can/skb.h#L130 >> >> > > My bad! > > Pointing you not reading the code ... should better have looked myself :-D >
| |