| Message ID | 20230208070759.462019-1-tung.q.nguyen@dektech.com.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net,1/1] tipc: fix kernel warning when sending SYN message | expand |
On Wed, 2023-02-08 at 07:07 +0000, Tung Nguyen wrote: > When sending a SYN message, this kernel stack trace is observed: > > ... > [ 13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550 > ... > [ 13.398494] Call Trace: > [ 13.398630] <TASK> > [ 13.398630] ? __alloc_skb+0xed/0x1a0 > [ 13.398630] tipc_msg_build+0x12c/0x670 [tipc] > [ 13.398630] ? shmem_add_to_page_cache.isra.71+0x151/0x290 > [ 13.398630] __tipc_sendmsg+0x2d1/0x710 [tipc] > [ 13.398630] ? tipc_connect+0x1d9/0x230 [tipc] > [ 13.398630] ? __local_bh_enable_ip+0x37/0x80 > [ 13.398630] tipc_connect+0x1d9/0x230 [tipc] > [ 13.398630] ? __sys_connect+0x9f/0xd0 > [ 13.398630] __sys_connect+0x9f/0xd0 > [ 13.398630] ? preempt_count_add+0x4d/0xa0 > [ 13.398630] ? fpregs_assert_state_consistent+0x22/0x50 > [ 13.398630] __x64_sys_connect+0x16/0x20 > [ 13.398630] do_syscall_64+0x42/0x90 > [ 13.398630] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > It is because commit a41dad905e5a ("iov_iter: saner checks for attempt > to copy to/from iterator") has introduced sanity check for copying > from/to iov iterator. Lacking of copy direction from the iterator > viewpoint would lead to kernel stack trace like above. > > This commit fixes this issue by initializing the iov iterator with > the correct copy direction. > > Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion") > Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com > Acked-by: Jon Maloy <jmaloy@redhat.com> > Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au> > --- > v2: add Fixes tag > > net/tipc/msg.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c > index 5c9fd4791c4b..cce118fea07a 100644 > --- a/net/tipc/msg.c > +++ b/net/tipc/msg.c > @@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset, > > msg_set_size(mhdr, msz); > > + if (!dsz) > + iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0); It looks like the root cause of the problem is that not all (indirect) callers of tipc_msg_build() properly initialize the iter. tipc_connect() is one of such caller, but AFAICS even tipc_accept() can reach tipc_msg_build() without proper iter initialization - via __tipc_sendstream -> __tipc_sendmsg. I think it's better if you address the issue in relevant callers, avoiding unneeded and confusing code in tipc_msg_build(). Thanks, Paolo
>-----Original Message----- >From: Paolo Abeni <pabeni@redhat.com> >Sent: Thursday, February 9, 2023 5:39 PM >To: Tung Quang Nguyen <tung.q.nguyen@dektech.com.au>; netdev@vger.kernel.org >Cc: davem@davemloft.net; kuba@kernel.org; edumazet@google.com; jmaloy@redhat.com; ying.xue@windriver.com; >viro@zeniv.linux.org.uk; syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com >Subject: Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message > >On Wed, 2023-02-08 at 07:07 +0000, Tung Nguyen wrote: >> When sending a SYN message, this kernel stack trace is observed: >> >> ... >> [ 13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550 >> ... >> [ 13.398494] Call Trace: >> [ 13.398630] <TASK> >> [ 13.398630] ? __alloc_skb+0xed/0x1a0 >> [ 13.398630] tipc_msg_build+0x12c/0x670 [tipc] >> [ 13.398630] ? shmem_add_to_page_cache.isra.71+0x151/0x290 >> [ 13.398630] __tipc_sendmsg+0x2d1/0x710 [tipc] >> [ 13.398630] ? tipc_connect+0x1d9/0x230 [tipc] >> [ 13.398630] ? __local_bh_enable_ip+0x37/0x80 >> [ 13.398630] tipc_connect+0x1d9/0x230 [tipc] >> [ 13.398630] ? __sys_connect+0x9f/0xd0 >> [ 13.398630] __sys_connect+0x9f/0xd0 >> [ 13.398630] ? preempt_count_add+0x4d/0xa0 >> [ 13.398630] ? fpregs_assert_state_consistent+0x22/0x50 >> [ 13.398630] __x64_sys_connect+0x16/0x20 >> [ 13.398630] do_syscall_64+0x42/0x90 >> [ 13.398630] entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> It is because commit a41dad905e5a ("iov_iter: saner checks for attempt >> to copy to/from iterator") has introduced sanity check for copying >> from/to iov iterator. Lacking of copy direction from the iterator >> viewpoint would lead to kernel stack trace like above. >> >> This commit fixes this issue by initializing the iov iterator with >> the correct copy direction. >> >> Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion") >> Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com >> Acked-by: Jon Maloy <jmaloy@redhat.com> >> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au> >> --- >> v2: add Fixes tag >> >> net/tipc/msg.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/tipc/msg.c b/net/tipc/msg.c >> index 5c9fd4791c4b..cce118fea07a 100644 >> --- a/net/tipc/msg.c >> +++ b/net/tipc/msg.c >> @@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset, >> >> msg_set_size(mhdr, msz); >> >> + if (!dsz) >> + iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0); > >It looks like the root cause of the problem is that not all (indirect) >callers of tipc_msg_build() properly initialize the iter. > >tipc_connect() is one of such caller, but AFAICS even tipc_accept() can >reach tipc_msg_build() without proper iter initialization - via >__tipc_sendstream -> __tipc_sendmsg. > >I think it's better if you address the issue in relevant callers, >avoiding unneeded and confusing code in tipc_msg_build(). I am fully aware of callers (without initializing iovec) of this function. My intention was to make as less change as possible. Do you think using iov_iter_kvec() instead in the callers make sense if I go for what you suggested ? > >Thanks, > >Paolo
On Thu, 9 Feb 2023 11:10:16 +0000 Tung Quang Nguyen wrote: > >> msg_set_size(mhdr, msz); > >> > >> + if (!dsz) > >> + iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0); > > > >It looks like the root cause of the problem is that not all (indirect) > >callers of tipc_msg_build() properly initialize the iter. > > > >tipc_connect() is one of such caller, but AFAICS even tipc_accept() can > >reach tipc_msg_build() without proper iter initialization - via > >__tipc_sendstream -> __tipc_sendmsg. > > > >I think it's better if you address the issue in relevant callers, > >avoiding unneeded and confusing code in tipc_msg_build(). > > I am fully aware of callers (without initializing iovec) of this > function. My intention was to make as less change as possible. General kernel guidance is to fix things "right" (i.e. so that the fix doesn't have to be refactored later). > Do you think using iov_iter_kvec() instead in the callers make sense > if I go for what you suggested ? I think so. These are the potential culprits? $ git grep 'struct msghdr [^*]*;' -- net/tipc/ net/tipc/socket.c: struct msghdr m = {NULL,}; net/tipc/socket.c: struct msghdr m = {NULL,}; net/tipc/topsrv.c: struct msghdr msg; net/tipc/topsrv.c: struct msghdr msg = {};
>-----Original Message----- >From: Jakub Kicinski <kuba@kernel.org> >Sent: Saturday, February 11, 2023 10:16 AM >To: Tung Quang Nguyen <tung.q.nguyen@dektech.com.au> >Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; >jmaloy@redhat.com; ying.xue@windriver.com; viro@zeniv.linux.org.uk; syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com >Subject: Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message > >On Thu, 9 Feb 2023 11:10:16 +0000 Tung Quang Nguyen wrote: >> >> msg_set_size(mhdr, msz); >> >> >> >> + if (!dsz) >> >> + iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0); >> > >> >It looks like the root cause of the problem is that not all (indirect) >> >callers of tipc_msg_build() properly initialize the iter. >> > >> >tipc_connect() is one of such caller, but AFAICS even tipc_accept() can >> >reach tipc_msg_build() without proper iter initialization - via >> >__tipc_sendstream -> __tipc_sendmsg. >> > >> >I think it's better if you address the issue in relevant callers, >> >avoiding unneeded and confusing code in tipc_msg_build(). >> >> I am fully aware of callers (without initializing iovec) of this >> function. My intention was to make as less change as possible. > >General kernel guidance is to fix things "right" (i.e. so that the fix >doesn't have to be refactored later). > >> Do you think using iov_iter_kvec() instead in the callers make sense >> if I go for what you suggested ? > >I think so. These are the potential culprits? > >$ git grep 'struct msghdr [^*]*;' -- net/tipc/ >net/tipc/socket.c: struct msghdr m = {NULL,}; >net/tipc/socket.c: struct msghdr m = {NULL,}; >net/tipc/topsrv.c: struct msghdr msg; >net/tipc/topsrv.c: struct msghdr msg = {}; Thanks. I will send v3.
diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 5c9fd4791c4b..cce118fea07a 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset, msg_set_size(mhdr, msz); + if (!dsz) + iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0); + /* No fragmentation needed? */ if (likely(msz <= pktmax)) { skb = tipc_buf_acquire(msz, GFP_KERNEL);
When sending a SYN message, this kernel stack trace is observed: ... [ 13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550 ... [ 13.398494] Call Trace: [ 13.398630] <TASK> [ 13.398630] ? __alloc_skb+0xed/0x1a0 [ 13.398630] tipc_msg_build+0x12c/0x670 [tipc] [ 13.398630] ? shmem_add_to_page_cache.isra.71+0x151/0x290 [ 13.398630] __tipc_sendmsg+0x2d1/0x710 [tipc] [ 13.398630] ? tipc_connect+0x1d9/0x230 [tipc] [ 13.398630] ? __local_bh_enable_ip+0x37/0x80 [ 13.398630] tipc_connect+0x1d9/0x230 [tipc] [ 13.398630] ? __sys_connect+0x9f/0xd0 [ 13.398630] __sys_connect+0x9f/0xd0 [ 13.398630] ? preempt_count_add+0x4d/0xa0 [ 13.398630] ? fpregs_assert_state_consistent+0x22/0x50 [ 13.398630] __x64_sys_connect+0x16/0x20 [ 13.398630] do_syscall_64+0x42/0x90 [ 13.398630] entry_SYSCALL_64_after_hwframe+0x63/0xcd It is because commit a41dad905e5a ("iov_iter: saner checks for attempt to copy to/from iterator") has introduced sanity check for copying from/to iov iterator. Lacking of copy direction from the iterator viewpoint would lead to kernel stack trace like above. This commit fixes this issue by initializing the iov iterator with the correct copy direction. Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion") Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com Acked-by: Jon Maloy <jmaloy@redhat.com> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au> --- v2: add Fixes tag net/tipc/msg.c | 3 +++ 1 file changed, 3 insertions(+)