| Message ID | 20220720110645.519601-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: j1939: Remove unnecessary WARN_ON_ONCE in j1939_sk_queue_activate_next_locked() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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 <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > > --- > 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 > >
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?
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 {
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 <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- net/can/j1939/socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)