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 <[email protected]>
> Cc: Devid Antonio Filoni <[email protected]>
> ---
> 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 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
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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 <[email protected]>
> > Cc: Devid Antonio Filoni <[email protected]>
> > ---
> > 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sat. 14 May 2022 at 12:29, Marc Kleine-Budde <[email protected]> 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