| Message ID | 20191218084355.24398-1-o.rempel@pengutronix.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1] can: j1939: transport: j1939_simple_recv(): ignore local J1939 messages send not by J1939 stack | expand |
Hi Oleksij, On 18/12/2019 09.43, Oleksij Rempel wrote: > In current J1939 stack implementation, we process all locally send > messages as own messages. Even if it was send by CAN_RAW socket. > > To reproduce it use following commands: > testj1939 -P -r can0:0x80 & > cansend can0 18238040#0123 > > This step will trigger false positive not critical warning: > j1939_simple_recv: Received already invalidated message > > With this patch we add additional check to make sure, related skb is own > echo message. in net/can/raw.c we check whether the CAN has been sent from that socket (an by default suppress our own transmitted data): https://elixir.bootlin.com/linux/v5.4.3/source/net/can/raw.c#L124 would checking against the 'sk' work for you too? What happens if someone runs a J1939 implementation on a CAN_RAW socket in addition to the in-kernel implementation? Can they talk to each other? Regards, Oliver > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > net/can/j1939/socket.c | 1 + > net/can/j1939/transport.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c > index f7587428febd..b9a17c2ee16f 100644 > --- a/net/can/j1939/socket.c > +++ b/net/can/j1939/socket.c > @@ -398,6 +398,7 @@ static int j1939_sk_init(struct sock *sk) > spin_lock_init(&jsk->sk_session_queue_lock); > INIT_LIST_HEAD(&jsk->sk_session_queue); > sk->sk_destruct = j1939_sk_sock_destruct; > + sk->sk_protocol = CAN_J1939; > > return 0; > } > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > index 9f99af5b0b11..b135c5e2a86e 100644 > --- a/net/can/j1939/transport.c > +++ b/net/can/j1939/transport.c > @@ -2017,6 +2017,10 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb) > if (!skb->sk) > return; > > + if (skb->sk->sk_family != AF_CAN || > + skb->sk->sk_protocol != CAN_J1939) > + return; > + > j1939_session_list_lock(priv); > session = j1939_session_get_simple(priv, skb); > j1939_session_list_unlock(priv); >
On Wed, Dec 18, 2019 at 10:03:27AM +0100, Oliver Hartkopp wrote: > Hi Oleksij, > > On 18/12/2019 09.43, Oleksij Rempel wrote: > > In current J1939 stack implementation, we process all locally send > > messages as own messages. Even if it was send by CAN_RAW socket. > > > > To reproduce it use following commands: > > testj1939 -P -r can0:0x80 & > > cansend can0 18238040#0123 > > > > This step will trigger false positive not critical warning: > > j1939_simple_recv: Received already invalidated message > > > > With this patch we add additional check to make sure, related skb is own > > echo message. > > in net/can/raw.c we check whether the CAN has been sent from that socket (an > by default suppress our own transmitted data): > > https://elixir.bootlin.com/linux/v5.4.3/source/net/can/raw.c#L124 > > would checking against the 'sk' work for you too? The J1939 stack work per interface. On each interface we have J1939 privat data with session list. For each new transfer we register a new session and wait for echo package to confirm the state of this session. On recv, if skb->sk is set, we assume, it is echo message, so we searching for related session to complete or continue with it. Since each session created on sendmsg path is bound to one of sockets, we won't be able to say if this skb->sk refers to valid but released socket. Or there is some kind of bug which need to be fixed. Searching for the session make sense only if message was handled/send by the local kernel j1939 stack. If there are other, for example user space j1939 implementations, we should handle them as remote not local stacks. So, there should be no difference to other j1939 devices on the CAN bus. J1939 kernel stack cares about echo messages to guarantee proper ordering of data and control packets on the bus. > > What happens if someone runs a J1939 implementation on a CAN_RAW socket in > addition to the in-kernel implementation? Can they talk to each other? Yes. This patch addressing exactly this use case. It is just eliminate false positive echo handling. Regards, Oleksij > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > net/can/j1939/socket.c | 1 + > > net/can/j1939/transport.c | 4 ++++ > > 2 files changed, 5 insertions(+) > > > > diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c > > index f7587428febd..b9a17c2ee16f 100644 > > --- a/net/can/j1939/socket.c > > +++ b/net/can/j1939/socket.c > > @@ -398,6 +398,7 @@ static int j1939_sk_init(struct sock *sk) > > spin_lock_init(&jsk->sk_session_queue_lock); > > INIT_LIST_HEAD(&jsk->sk_session_queue); > > sk->sk_destruct = j1939_sk_sock_destruct; > > + sk->sk_protocol = CAN_J1939; > > return 0; > > } > > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > > index 9f99af5b0b11..b135c5e2a86e 100644 > > --- a/net/can/j1939/transport.c > > +++ b/net/can/j1939/transport.c > > @@ -2017,6 +2017,10 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb) > > if (!skb->sk) > > return; > > + if (skb->sk->sk_family != AF_CAN || > > + skb->sk->sk_protocol != CAN_J1939) > > + return; > > + > > j1939_session_list_lock(priv); > > session = j1939_session_get_simple(priv, skb); > > j1939_session_list_unlock(priv); > > > >
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c index f7587428febd..b9a17c2ee16f 100644 --- a/net/can/j1939/socket.c +++ b/net/can/j1939/socket.c @@ -398,6 +398,7 @@ static int j1939_sk_init(struct sock *sk) spin_lock_init(&jsk->sk_session_queue_lock); INIT_LIST_HEAD(&jsk->sk_session_queue); sk->sk_destruct = j1939_sk_sock_destruct; + sk->sk_protocol = CAN_J1939; return 0; } diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index 9f99af5b0b11..b135c5e2a86e 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -2017,6 +2017,10 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb) if (!skb->sk) return; + if (skb->sk->sk_family != AF_CAN || + skb->sk->sk_protocol != CAN_J1939) + return; + j1939_session_list_lock(priv); session = j1939_session_get_simple(priv, skb); j1939_session_list_unlock(priv);
In current J1939 stack implementation, we process all locally send messages as own messages. Even if it was send by CAN_RAW socket. To reproduce it use following commands: testj1939 -P -r can0:0x80 & cansend can0 18238040#0123 This step will trigger false positive not critical warning: j1939_simple_recv: Received already invalidated message With this patch we add additional check to make sure, related skb is own echo message. Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- net/can/j1939/socket.c | 1 + net/can/j1939/transport.c | 4 ++++ 2 files changed, 5 insertions(+)