[PATCH 1/1] can: skb: add and set local_origin flag

[PATCH 1/1] can: skb: add and set local_origin flag

[Oleksij Rempel]

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;
 
 	*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;
-- 
2.30.2

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Oliver Hartkopp]

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;

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Oleksij Rempel]

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
-- 
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 |

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Oliver Hartkopp]

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

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Oliver Hartkopp]

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
> 

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Oliver Hartkopp]

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

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[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
> 

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] can: skb: add and set local_origin flag

[Vincent Mailhol]

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