The purpose of WARN_ON_ONCE if the session with the same parameters
has already been activated and is currently in active_session_list is
not very clear. Is this warning implemented to indicate that userspace
is doing something wrong?
As far as I can see, there are two lists: active_session_list (which
is for the whole device) and sk_session_queue (which is unique for
each j1939_sock), and the situation when we have two sessions with
the same type, addresses and destinations in two different
sk_session_queues (owned by two different sockets) is actually highly
probable - one is active and the other is willing to become active
but the j1939_session_activate() does not let that happen. It is
correct behaviour as I assume.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
net/can/j1939/socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index f5ecfdcf57b2..be4b73afa16c 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -178,7 +178,7 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
if (!first)
return;
- if (WARN_ON_ONCE(j1939_session_activate(first))) {
+ if (j1939_session_activate(first)) {
first->err = -EBUSY;
goto activate_next;
} else {
--
2.25.1
Hi Fedor,
On Wed, Jul 20, 2022 at 02:06:45PM +0300, Fedor Pchelkin wrote:
> The purpose of WARN_ON_ONCE if the session with the same parameters
> has already been activated and is currently in active_session_list is
> not very clear. Is this warning implemented to indicate that userspace
> is doing something wrong?
yes.
> As far as I can see, there are two lists: active_session_list (which
> is for the whole device) and sk_session_queue (which is unique for
> each j1939_sock), and the situation when we have two sessions with
> the same type, addresses and destinations in two different
> sk_session_queues (owned by two different sockets) is actually highly
> probable - one is active and the other is willing to become active
> but the j1939_session_activate() does not let that happen. It is
> correct behaviour as I assume.
No. It is not typical use case and most probably it will create
problems. Are you working on some system where this use case is valid?
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
>
> ---
> net/can/j1939/socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index f5ecfdcf57b2..be4b73afa16c 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -178,7 +178,7 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
> if (!first)
> return;
>
> - if (WARN_ON_ONCE(j1939_session_activate(first))) {
> + if (j1939_session_activate(first)) {
> first->err = -EBUSY;
> goto activate_next;
> } else {
> --
> 2.25.1
>
>
--
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 |
Hello Oleksij,
I'm sorry for late answering.
On 20.07.2022 22:13, Oleksij Rempel wrote:
>> Are you working on some system where this use case is valid?
No, we are fuzzing the kernel and analyzing different warnings and
crashes.
On 20.07.2022 22:13, Oleksij Rempel wrote:
> yes
Well, there is a long story about where and for which purposes the
kernel warning macros should be correctly used and, overall,
WARN_ON_ONCE is not intended for user-space notification.
Linus Torvalds wrote:
> WARN_ON() should only be used for "This cannot happen, but if it does,
> I want to know how we got here".
>
> So if that j1939 thing is something that can be triggered by a user,
> then the backtrace should be reported to the driver maintainer, and
> then either
>
> (a) the WARN_ON_ONCE() should just be removed ("ok, this can happen,
> we understand why it can happen, and it's fine")
>
> (b) the problem the WARN_ON_ONCE() reports about should be made
> impossible some way
>
> (c) it might be downgraded to a pr_warn() if people really want to
> tell user space that "guys, you're doing something wrong" and it's
> considered a useful warning.
So WARN_ON_ONCE should be replaced with a more gentle variant - I think
pr_warn_once would suit this case. I've prepared a new patch for that,
it will follow this email.
Could you also look at the patch - [PATCH] can: j1939: fix memory leak
of skbs - which I sent you on 08.07.2022, please?
We should warn user-space that it is doing something wrong when trying to
activate sessions with identical parameters but WARN_ON_ONCE macro can not
be used here as it serves a different purpose.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
net/can/j1939/socket.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index f5ecfdcf57b2..67e8b50b8bc1 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -178,7 +178,8 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
if (!first)
return;
- if (WARN_ON_ONCE(j1939_session_activate(first))) {
+ if (j1939_session_activate(first)) {
+ pr_warn_once("can: j1939: Identical session is already activated.\n");
first->err = -EBUSY;
goto activate_next;
} else {
--
2.25.1
Hi Fedor,
thank you for your patch.
On Thu, Jul 28, 2022 at 07:34:29PM +0300, Fedor Pchelkin wrote:
> We should warn user-space that it is doing something wrong when trying to
> activate sessions with identical parameters but WARN_ON_ONCE macro can not
> be used here as it serves a different purpose.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> ---
> net/can/j1939/socket.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index f5ecfdcf57b2..67e8b50b8bc1 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -178,7 +178,8 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
> if (!first)
> return;
>
> - if (WARN_ON_ONCE(j1939_session_activate(first))) {
> + if (j1939_session_activate(first)) {
> + pr_warn_once("can: j1939: Identical session is already activated.\n");
please use netdev_warn_once().
Otherwise looks good.
> first->err = -EBUSY;
> goto activate_next;
> } else {
> --
> 2.25.1
>
>
--
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 |
We should warn user-space that it is doing something wrong when trying to
activate sessions with identical parameters but WARN_ON_ONCE macro can not
be used here as it serves a different purpose.
So it would be good to replace it with netdev_warn_once() message.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
v1 -> v2: Used netdev_warn_once() instead of pr_warn_once()
net/can/j1939/socket.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index f5ecfdcf57b2..09e1d78bd22c 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -178,7 +178,10 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
if (!first)
return;
- if (WARN_ON_ONCE(j1939_session_activate(first))) {
+ if (j1939_session_activate(first)) {
+ netdev_warn_once(first->priv->ndev,
+ "%s: 0x%p: Identical session is already activated.\n",
+ __func__, first);
first->err = -EBUSY;
goto activate_next;
} else {
--
2.25.1
On Fri, Jul 29, 2022 at 05:06:09PM +0300, Fedor Pchelkin wrote:
> We should warn user-space that it is doing something wrong when trying to
> activate sessions with identical parameters but WARN_ON_ONCE macro can not
> be used here as it serves a different purpose.
>
> So it would be good to replace it with netdev_warn_once() message.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
Acked-by: Oleksij Rempel <[email protected]>
> ---
> v1 -> v2: Used netdev_warn_once() instead of pr_warn_once()
>
> net/can/j1939/socket.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index f5ecfdcf57b2..09e1d78bd22c 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -178,7 +178,10 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
> if (!first)
> return;
>
> - if (WARN_ON_ONCE(j1939_session_activate(first))) {
> + if (j1939_session_activate(first)) {
> + netdev_warn_once(first->priv->ndev,
> + "%s: 0x%p: Identical session is already activated.\n",
> + __func__, first);
> first->err = -EBUSY;
> goto activate_next;
> } else {
> --
> 2.25.1
>
>
--
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 |
We should warn user-space that it is doing something wrong when trying to
activate sessions with identical parameters but WARN_ON_ONCE macro can not
be used here as it serves a different purpose.
So it would be good to replace it with netdev_warn_once() message.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
Acked-by: Oleksij Rempel <[email protected]>
---
v1 -> v2: Used netdev_warn_once() instead of pr_warn_once()
net/can/j1939/socket.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index f5ecfdcf57b2..09e1d78bd22c 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -178,7 +178,10 @@ static void j1939_sk_queue_activate_next_locked(struct j1939_session *session)
if (!first)
return;
- if (WARN_ON_ONCE(j1939_session_activate(first))) {
+ if (j1939_session_activate(first)) {
+ netdev_warn_once(first->priv->ndev,
+ "%s: 0x%p: Identical session is already activated.\n",
+ __func__, first);
first->err = -EBUSY;
goto activate_next;
} else {
--
2.25.1
On 29.07.2022 17:36:55, Fedor Pchelkin wrote:
> We should warn user-space that it is doing something wrong when trying to
> activate sessions with identical parameters but WARN_ON_ONCE macro can not
> be used here as it serves a different purpose.
>
> So it would be good to replace it with netdev_warn_once() message.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> Acked-by: Oleksij Rempel <[email protected]>
Nitpick: You should add your S-o-b below every other tag line.
Added to linux-can with fixed indention.
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 |