[PATCH] can: raw: fix TX CAN frames show up as RX'ed ones

[PATCH] can: raw: fix TX CAN frames show up as RX'ed ones

[Marc Kleine-Budde]

Fixes: e940e0895a82 ("can: skb: can_skb_set_owner(): fix ref counting if socket was closed before setting skb ownership")
Reported-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hey Torin,

this is only compile time tested. Please give it a try.

regards,
Marc

 drivers/net/can/dev/skb.c |  2 ++
 include/linux/can/skb.h   | 14 +++++++++++---
 net/can/raw.c             |  2 +-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 61660248c69e..9644c0d85bb6 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -200,6 +200,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)->flags = 0;
 
 	*cf = skb_put_zero(skb, sizeof(struct can_frame));
 
@@ -231,6 +232,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)->flags = 0;
 
 	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index d311bc369a39..24a3e682b4a2 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -51,6 +51,7 @@ struct can_skb_priv {
 	int ifindex;
 	int skbcnt;
 	unsigned int frame_len;
+	unsigned int flags;
 	struct can_frame cf[];
 };
 
@@ -71,9 +72,16 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
 	 * after the last TX skb has been freed). So only increase
 	 * socket refcount if the refcount is > 0.
 	 */
-	if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
-		skb->destructor = sock_efree;
-		skb->sk = sk;
+	if (sk) {
+		struct can_skb_priv *skb_priv;
+
+		skb_priv = can_skb_prv(skb);
+		skb_priv->flags = MSG_DONTROUTE;
+
+		if (refcount_inc_not_zero(&sk->sk_refcnt)) {
+			skb->destructor = sock_efree;
+			skb->sk = sk;
+		}
 	}
 }
 
diff --git a/net/can/raw.c b/net/can/raw.c
index 139d9471ddcf..9bedd0672fae 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -169,7 +169,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(oskb)->flags == MSG_DONTROUTE)
 		*pflags |= MSG_DONTROUTE;
 	if (oskb->sk == sk)
 		*pflags |= MSG_CONFIRM;
-- 
2.30.2

Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones

[Torin Cooper-Bennun]

On Mon, May 10, 2021 at 08:20:39PM +0200, Marc Kleine-Budde wrote:
> Fixes: e940e0895a82 ("can: skb: can_skb_set_owner(): fix ref counting if socket was closed before setting skb ownership")
> Reported-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> Hey Torin,
> 
> this is only compile time tested. Please give it a try.
> 
> regards,
> Marc
> 
>  drivers/net/can/dev/skb.c |  2 ++
>  include/linux/can/skb.h   | 14 +++++++++++---
>  net/can/raw.c             |  2 +-
>  3 files changed, 14 insertions(+), 4 deletions(-)

Thanks Marc, works beautifully here.

Tested-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones

[Marc Kleine-Budde]

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

On 11.05.2021 11:00:08, Torin Cooper-Bennun wrote:
> On Mon, May 10, 2021 at 08:20:39PM +0200, Marc Kleine-Budde wrote:
> > Fixes: e940e0895a82 ("can: skb: can_skb_set_owner(): fix ref counting if socket was closed before setting skb ownership")
> > Reported-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> > Hey Torin,
> > 
> > this is only compile time tested. Please give it a try.
> > 
> > regards,
> > Marc
> > 
> >  drivers/net/can/dev/skb.c |  2 ++
> >  include/linux/can/skb.h   | 14 +++++++++++---
> >  net/can/raw.c             |  2 +-
> >  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> Thanks Marc, works beautifully here.
> 
> Tested-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>

\o/ ignore my last mail \o/

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] can: raw: fix TX CAN frames show up as RX'ed ones

[Marc Kleine-Budde]

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

On 10.05.2021 20:20:39, Marc Kleine-Budde wrote:
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 61660248c69e..9644c0d85bb6 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -200,6 +200,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)->flags = 0;
>  
>  	*cf = skb_put_zero(skb, sizeof(struct can_frame));
>  
> @@ -231,6 +232,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)->flags = 0;
>  
>  	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
>  
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index d311bc369a39..24a3e682b4a2 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -51,6 +51,7 @@ struct can_skb_priv {
>  	int ifindex;
>  	int skbcnt;
>  	unsigned int frame_len;
> +	unsigned int flags;
>  	struct can_frame cf[];
>  };
>  
> @@ -71,9 +72,16 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
>  	 * after the last TX skb has been freed). So only increase
>  	 * socket refcount if the refcount is > 0.
>  	 */
> -	if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
                  ^^^^^^^^^^^^^^^^^^^^^
> -		skb->destructor = sock_efree;
> -		skb->sk = sk;

This is the problem:

It was introduced by Oleksij and me in e940e0895a82 ("can: skb:
can_skb_set_owner(): fix ref counting if socket was closed before
setting skb ownership").

In e940e0895a82 we fix the ref counting on the socket problem, as
described in the comment of that patch:

|  static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
|  {
| -       if (sk) {
| -               sock_hold(sk);
| +       /* If the socket has already been closed by user space, the
| +        * refcount may already be 0 (and the socket will be freed
| +        * after the last TX skb has been freed). So only increase
| +        * socket refcount if the refcount is > 0.
| +        */
| +       if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
|                 skb->destructor = sock_efree;
|                 skb->sk = sk;
|         }

We missed the fact that skb->sk is used in raw_rcv() to determine if the
CAN frame was received from outside or it was the echo of a TX skb. As a
workaround/fix I introduced a new variable "flags" to the struct
can_skb_priv and added MSG_DONTROUTE directly there, if a socket was
associated with the skb. I haven't tested what that means for vxcan
devices in network name spaces.

> +	if (sk) {
> +		struct can_skb_priv *skb_priv;
> +
> +		skb_priv = can_skb_prv(skb);
> +		skb_priv->flags = MSG_DONTROUTE;
> +
> +		if (refcount_inc_not_zero(&sk->sk_refcnt)) {
> +			skb->destructor = sock_efree;
> +			skb->sk = sk;
> +		}
>  	}
>  }
>  
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 139d9471ddcf..9bedd0672fae 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -169,7 +169,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(oskb)->flags == MSG_DONTROUTE)
>  		*pflags |= MSG_DONTROUTE;
>  	if (oskb->sk == sk)
>  		*pflags |= MSG_CONFIRM;

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] can: raw: fix TX CAN frames show up as RX'ed ones

[Joakim Zhang]

Hi Marc,

I have not seen this patch in linux-can repo, what the status? Thanks.

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2021年5月11日 2:21
> To: linux-can@vger.kernel.org
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>; Torin Cooper-Bennun
> <torin@maxiluxsystems.com>
> Subject: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
> 
> Fixes: e940e0895a82 ("can: skb: can_skb_set_owner(): fix ref counting if socket
> was closed before setting skb ownership")
> Reported-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> Hey Torin,
> 
> this is only compile time tested. Please give it a try.
> 
> regards,
> Marc
> 
>  drivers/net/can/dev/skb.c |  2 ++
>  include/linux/can/skb.h   | 14 +++++++++++---
>  net/can/raw.c             |  2 +-
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index
> 61660248c69e..9644c0d85bb6 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -200,6 +200,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)->flags = 0;
> 
>  	*cf = skb_put_zero(skb, sizeof(struct can_frame));
> 
> @@ -231,6 +232,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)->flags = 0;
> 
>  	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
> 
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h index
> d311bc369a39..24a3e682b4a2 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -51,6 +51,7 @@ struct can_skb_priv {
>  	int ifindex;
>  	int skbcnt;
>  	unsigned int frame_len;
> +	unsigned int flags;
>  	struct can_frame cf[];
>  };
> 
> @@ -71,9 +72,16 @@ static inline void can_skb_set_owner(struct sk_buff
> *skb, struct sock *sk)
>  	 * after the last TX skb has been freed). So only increase
>  	 * socket refcount if the refcount is > 0.
>  	 */
> -	if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
> -		skb->destructor = sock_efree;
> -		skb->sk = sk;
> +	if (sk) {
> +		struct can_skb_priv *skb_priv;
> +
> +		skb_priv = can_skb_prv(skb);
> +		skb_priv->flags = MSG_DONTROUTE;
> +
> +		if (refcount_inc_not_zero(&sk->sk_refcnt)) {
> +			skb->destructor = sock_efree;
> +			skb->sk = sk;
> +		}
>  	}
>  }
> 
> diff --git a/net/can/raw.c b/net/can/raw.c index 139d9471ddcf..9bedd0672fae
> 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -169,7 +169,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(oskb)->flags == MSG_DONTROUTE)
>  		*pflags |= MSG_DONTROUTE;
>  	if (oskb->sk == sk)
>  		*pflags |= MSG_CONFIRM;
> --
> 2.30.2
> 

Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones

[Marc Kleine-Budde]

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

On 27.05.2021 07:17:22, Joakim Zhang wrote:
> I have not seen this patch in linux-can repo, what the status? Thanks.

I still want to test the patch with network namespaces. I think I'll do
this now.

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] can: raw: fix TX CAN frames show up as RX'ed ones

[Torin Cooper-Bennun]

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

On Thu, May 27, 2021 at 10:54:25AM +0200, Marc Kleine-Budde wrote:
> On 27.05.2021 07:17:22, Joakim Zhang wrote:
> > I have not seen this patch in linux-can repo, what the status? Thanks.
> 
> I still want to test the patch with network namespaces. I think I'll do
> this now.

Hi Marc. Did any testing go ahead on this? I'm happy to use this patch
manually but it would be cool to have it mainline and backported if/when
possible! :)

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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