| Message ID | 20220822040628.177649-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] kcm: get rid of unnecessary cleanup | expand |
On Sun, 21 Aug 2022 21:06:28 -0700 Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > strp_init() is called just a few lines above this csk->sk_user_data > check, it also initializes strp->work etc., therefore, it is > unnecessary to call strp_done() to cancel the freshly initialized > work. > > This also makes a lockdep warning reported by syzbot go away. > > Reported-and-tested-by: syzbot+9fc084a4348493ef65d2@syzkaller.appspotmail.com > Reported-by: syzbot+e696806ef96cdd2d87cd@syzkaller.appspotmail.com > Fixes: e5571240236c ("kcm: Check if sk_user_data already set in kcm_attach") > Fixes: dff8baa26117 ("kcm: Call strp_stop before strp_done in kcm_attach") > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Tom Herbert <tom@herbertland.com> > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > --- > net/kcm/kcmsock.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c > index 71899e5a5a11..661c40cdab3e 100644 > --- a/net/kcm/kcmsock.c > +++ b/net/kcm/kcmsock.c > @@ -1425,8 +1425,6 @@ static int kcm_attach(struct socket *sock, struct socket *csock, > */ > if (csk->sk_user_data) { > write_unlock_bh(&csk->sk_callback_lock); > - strp_stop(&psock->strp); > - strp_done(&psock->strp); > kmem_cache_free(kcm_psockp, psock); > err = -EALREADY; > goto out; Looks correct, but if strp_init() ever grows code which needs to be unwound in strp_done() we'd risk a leak. This seems to have been Tom's intent. Could we perhaps reorder the sk_user_data check vs the strp_init() call? sock_map already calls strp_init() under the callback lock so we'd not be introducing any new lock ordering.
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c index 71899e5a5a11..661c40cdab3e 100644 --- a/net/kcm/kcmsock.c +++ b/net/kcm/kcmsock.c @@ -1425,8 +1425,6 @@ static int kcm_attach(struct socket *sock, struct socket *csock, */ if (csk->sk_user_data) { write_unlock_bh(&csk->sk_callback_lock); - strp_stop(&psock->strp); - strp_done(&psock->strp); kmem_cache_free(kcm_psockp, psock); err = -EALREADY; goto out;