| Message ID | 20220511121913.2696181-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] can: skb: add and set local_origin flag | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Oleksij, On 5/11/22 14:19, Oleksij Rempel wrote: > Add new can_skb_priv::local_origin flag to be able detect egress > packages even if they was sent directly from kernel and not assigned to > some socket. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > Cc: Devid Antonio Filoni <devid.filoni@egluetechnologies.com> > --- > drivers/net/can/dev/skb.c | 3 +++ > include/linux/can/skb.h | 1 + > net/can/raw.c | 2 +- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c > index 61660248c69e..3e2357fb387e 100644 > --- a/drivers/net/can/dev/skb.c > +++ b/drivers/net/can/dev/skb.c > @@ -63,6 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > > /* save frame_len to reuse it when transmission is completed */ > can_skb_prv(skb)->frame_len = frame_len; > + can_skb_prv(skb)->local_origin = true; > > skb_tx_timestamp(skb); > > @@ -200,6 +201,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) > can_skb_reserve(skb); > can_skb_prv(skb)->ifindex = dev->ifindex; > can_skb_prv(skb)->skbcnt = 0; > + can_skb_prv(skb)->local_origin = false; > > *cf = skb_put_zero(skb, sizeof(struct can_frame)); > > @@ -231,6 +233,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, > can_skb_reserve(skb); > can_skb_prv(skb)->ifindex = dev->ifindex; > can_skb_prv(skb)->skbcnt = 0; > + can_skb_prv(skb)->local_origin = false; 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(). E.g. to make it work for virtual CAN and vxcan interfaces. I'm a bit unsure why we should not stick with the simple skb->sk handling? Regards, Oliver > > *cfd = skb_put_zero(skb, sizeof(struct canfd_frame)); > > diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h > index fdb22b00674a..1b8a8cf2b13b 100644 > --- a/include/linux/can/skb.h > +++ b/include/linux/can/skb.h > @@ -52,6 +52,7 @@ struct can_skb_priv { > int ifindex; > int skbcnt; > unsigned int frame_len; > + bool local_origin; > struct can_frame cf[]; > }; > > diff --git a/net/can/raw.c b/net/can/raw.c > index b7dbb57557f3..df2d9334b395 100644 > --- a/net/can/raw.c > +++ b/net/can/raw.c > @@ -173,7 +173,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data) > /* add CAN specific message flags for raw_recvmsg() */ > pflags = raw_flags(skb); > *pflags = 0; > - if (oskb->sk) > + if (can_skb_prv(skb)->local_origin) > *pflags |= MSG_DONTROUTE; > if (oskb->sk == sk) > *pflags |= MSG_CONFIRM;
On Wed, May 11, 2022 at 02:38:32PM +0200, Oliver Hartkopp wrote: > Hi Oleksij, > > On 5/11/22 14:19, Oleksij Rempel wrote: > > Add new can_skb_priv::local_origin flag to be able detect egress > > packages even if they was sent directly from kernel and not assigned to > > some socket. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > Cc: Devid Antonio Filoni <devid.filoni@egluetechnologies.com> > > --- > > drivers/net/can/dev/skb.c | 3 +++ > > include/linux/can/skb.h | 1 + > > net/can/raw.c | 2 +- > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c > > index 61660248c69e..3e2357fb387e 100644 > > --- a/drivers/net/can/dev/skb.c > > +++ b/drivers/net/can/dev/skb.c > > @@ -63,6 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > > /* save frame_len to reuse it when transmission is completed */ > > can_skb_prv(skb)->frame_len = frame_len; > > + can_skb_prv(skb)->local_origin = true; > > skb_tx_timestamp(skb); > > @@ -200,6 +201,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) > > can_skb_reserve(skb); > > can_skb_prv(skb)->ifindex = dev->ifindex; > > can_skb_prv(skb)->skbcnt = 0; > > + can_skb_prv(skb)->local_origin = false; > > *cf = skb_put_zero(skb, sizeof(struct can_frame)); > > @@ -231,6 +233,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, > > can_skb_reserve(skb); > > can_skb_prv(skb)->ifindex = dev->ifindex; > > can_skb_prv(skb)->skbcnt = 0; > > + can_skb_prv(skb)->local_origin = false; > > 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(). > > E.g. to make it work for virtual CAN and vxcan interfaces. ok, i'll take a look on it. > > I'm a bit unsure why we should not stick with the simple skb->sk handling? In case of J1939 we have kernel generate control frames not associated with any socket. For example transfer abort messages because no receive socket was detected. Or there are multiple receive sockets and attaching to one of it make no sense. Regards, Oleksij
On 11.05.2022 14:38:32, Oliver Hartkopp wrote: > I'm a bit unsure why we should not stick with the simple skb->sk > handling? Another use where skb->sk breaks is sending CAN frames with SO_TXTIME with the sched_etf. I have a patched version of cangen that uses SO_TXTIME. It attaches a time to transmit to a CAN frame when sending it. If you send 10 frames, each 100ms after the other and then exit the program, the first CAN frames show up as TX'ed while the others (after closing the socket) show up as RX'ed CAN frames in candump. regards, Marc
On 5/11/22 15:05, Marc Kleine-Budde wrote: > On 11.05.2022 14:38:32, Oliver Hartkopp wrote: >> I'm a bit unsure why we should not stick with the simple skb->sk >> handling? > > Another use where skb->sk breaks is sending CAN frames with SO_TXTIME > with the sched_etf. > > I have a patched version of cangen that uses SO_TXTIME. It attaches a > time to transmit to a CAN frame when sending it. If you send 10 frames, > each 100ms after the other and then exit the program, the first CAN > frames show up as TX'ed while the others (after closing the socket) show > up as RX'ed CAN frames in candump. Hm, this could be an argument for the origin flag. But I'm more scared about your described behaviour. What happens if the socket is still open? There obviously must be some instance removing the sk reference, right?? Regards, Oliver
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 Marc
On 11.05.2022 15:24:00, Oliver Hartkopp wrote: > > Another use where skb->sk breaks is sending CAN frames with SO_TXTIME > > with the sched_etf. > > > > I have a patched version of cangen that uses SO_TXTIME. It attaches a > > time to transmit to a CAN frame when sending it. If you send 10 frames, > > each 100ms after the other and then exit the program, the first CAN > > frames show up as TX'ed while the others (after closing the socket) show > > up as RX'ed CAN frames in candump. > > Hm, this could be an argument for the origin flag. > > But I'm more scared about your described behaviour. What happens if > the socket is still open? SO_TXTIME makes an existing race window really, really, really wide. The race window is between sendmsg() returning to user space and can_put_echo_skb() -> can_create_echo_skb() -> can_skb_set_owner(). In can_skb_set_owner() a reference to the socket is taken, if the socket is not closed: | https://elixir.bootlin.com/linux/v5.17.6/source/include/linux/can/skb.h#L75 If the socket closes _after_ sendmsg(), but _before_ the driver calls can_put_echo_skb() the CAN frame will have no socket reference and show up as RX'ed. > There obviously must be some instance removing the sk reference, right?? No. That's all fine. We fixes that in: | https://lore.kernel.org/all/20210226092456.27126-1-o.rempel@pengutronix.de/ Marc
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: | 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; + /* If the interface is not capable to do loopback * itself, we do it here. */ Marc
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: > > | 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; > + Good point! But please check the rest of the code. You need 'goto inval_skb;' instead of the return ;-) Best, Oliver > /* If the interface is not capable to do loopback > * itself, we do it here. > */ > > Marc >
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: > > > > | 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; > > + > > 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 Marc
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: >>> >>> | 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; >>> + >> >> 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
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 >
On 12.05.2022 08:23:26, Oliver Hartkopp wrote: > > > > > 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. ACK - I haven't checked, but I assume that all current callers of can_send() are sound, this I why I started the description with: "Assume an invalid CAN frame is passed to can_send()". But we can argue that we trust all callers. regards, Marc
On Sat. 14 May 2022 at 12:29, Marc Kleine-Budde <mkl@pengutronix.de> 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: > > | 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; > + This means that can_dropped_invalid_skb() would be called twice: one time in can_send() and one time in the driver's xmit() function, right? It would be nice to find a trick to detect whether the skb was injected through the packet socket or not in order not to execute can_dropped_invalid_skb() twice. I guess the information of the provenance of the skb is available somewhere, just not sure where (not familiar with the packet socket). Yours sincerely, Vincent Mailhol
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 61660248c69e..3e2357fb387e 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -63,6 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, /* save frame_len to reuse it when transmission is completed */ can_skb_prv(skb)->frame_len = frame_len; + can_skb_prv(skb)->local_origin = true; skb_tx_timestamp(skb); @@ -200,6 +201,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) can_skb_reserve(skb); can_skb_prv(skb)->ifindex = dev->ifindex; can_skb_prv(skb)->skbcnt = 0; + can_skb_prv(skb)->local_origin = false; *cf = skb_put_zero(skb, sizeof(struct can_frame)); @@ -231,6 +233,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, can_skb_reserve(skb); can_skb_prv(skb)->ifindex = dev->ifindex; can_skb_prv(skb)->skbcnt = 0; + can_skb_prv(skb)->local_origin = false; *cfd = skb_put_zero(skb, sizeof(struct canfd_frame)); diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h index fdb22b00674a..1b8a8cf2b13b 100644 --- a/include/linux/can/skb.h +++ b/include/linux/can/skb.h @@ -52,6 +52,7 @@ struct can_skb_priv { int ifindex; int skbcnt; unsigned int frame_len; + bool local_origin; struct can_frame cf[]; }; diff --git a/net/can/raw.c b/net/can/raw.c index b7dbb57557f3..df2d9334b395 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -173,7 +173,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data) /* add CAN specific message flags for raw_recvmsg() */ pflags = raw_flags(skb); *pflags = 0; - if (oskb->sk) + if (can_skb_prv(skb)->local_origin) *pflags |= MSG_DONTROUTE; if (oskb->sk == sk) *pflags |= MSG_CONFIRM;
Add new can_skb_priv::local_origin flag to be able detect egress packages even if they was sent directly from kernel and not assigned to some socket. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Cc: Devid Antonio Filoni <devid.filoni@egluetechnologies.com> --- drivers/net/can/dev/skb.c | 3 +++ include/linux/can/skb.h | 1 + net/can/raw.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-)