* [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
@ 2021-05-10 18:20 Marc Kleine-Budde
2021-05-11 10:00 ` Torin Cooper-Bennun
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2021-05-10 18:20 UTC (permalink / raw)
To: linux-can; +Cc: Marc Kleine-Budde, Torin Cooper-Bennun
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-10 18:20 [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones Marc Kleine-Budde
@ 2021-05-11 10:00 ` Torin Cooper-Bennun
2021-05-11 10:02 ` Marc Kleine-Budde
2021-05-11 10:39 ` Marc Kleine-Budde
2021-05-27 7:17 ` Joakim Zhang
2 siblings, 1 reply; 7+ messages in thread
From: Torin Cooper-Bennun @ 2021-05-11 10:00 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-11 10:00 ` Torin Cooper-Bennun
@ 2021-05-11 10:02 ` Marc Kleine-Budde
0 siblings, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2021-05-11 10:02 UTC (permalink / raw)
To: Torin Cooper-Bennun; +Cc: linux-can
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-10 18:20 [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones Marc Kleine-Budde
2021-05-11 10:00 ` Torin Cooper-Bennun
@ 2021-05-11 10:39 ` Marc Kleine-Budde
2021-05-27 7:17 ` Joakim Zhang
2 siblings, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2021-05-11 10:39 UTC (permalink / raw)
To: linux-can, Oliver Hartkopp; +Cc: Torin Cooper-Bennun
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-10 18:20 [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones Marc Kleine-Budde
2021-05-11 10:00 ` Torin Cooper-Bennun
2021-05-11 10:39 ` Marc Kleine-Budde
@ 2021-05-27 7:17 ` Joakim Zhang
2021-05-27 8:54 ` Marc Kleine-Budde
2 siblings, 1 reply; 7+ messages in thread
From: Joakim Zhang @ 2021-05-27 7:17 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: Torin Cooper-Bennun
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-27 7:17 ` Joakim Zhang
@ 2021-05-27 8:54 ` Marc Kleine-Budde
2021-07-21 7:59 ` Torin Cooper-Bennun
0 siblings, 1 reply; 7+ messages in thread
From: Marc Kleine-Budde @ 2021-05-27 8:54 UTC (permalink / raw)
To: Joakim Zhang; +Cc: linux-can, Torin Cooper-Bennun
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones
2021-05-27 8:54 ` Marc Kleine-Budde
@ 2021-07-21 7:59 ` Torin Cooper-Bennun
0 siblings, 0 replies; 7+ messages in thread
From: Torin Cooper-Bennun @ 2021-07-21 7:59 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Joakim Zhang, linux-can, 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-21 8:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 18:20 [PATCH] can: raw: fix TX CAN frames show up as RX'ed ones Marc Kleine-Budde
2021-05-11 10:00 ` Torin Cooper-Bennun
2021-05-11 10:02 ` Marc Kleine-Budde
2021-05-11 10:39 ` Marc Kleine-Budde
2021-05-27 7:17 ` Joakim Zhang
2021-05-27 8:54 ` Marc Kleine-Budde
2021-07-21 7:59 ` Torin Cooper-Bennun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.