| Message ID | YC88acS6dN6cU1y0@zeniv-ca.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] af_unix: take address assignment/hash insertion into a new helper | expand |
On Thu, Feb 18, 2021 at 8:22 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > Duplicated logics in all bind variants (autobind, bind-to-path, > bind-to-abstract) gets taken into a common helper. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > net/unix/af_unix.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 41c3303c3357..179b4fe837e6 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -262,6 +262,16 @@ static void __unix_insert_socket(struct hlist_head *list, struct sock *sk) > sk_add_node(sk, list); > } > > +static void __unix_set_addr(struct sock *sk, struct unix_address *addr, > + unsigned hash) > + __releases(&unix_table_lock) > +{ > + __unix_remove_socket(sk); > + smp_store_release(&unix_sk(sk)->addr, addr); > + __unix_insert_socket(&unix_socket_table[hash], sk); > + spin_unlock(&unix_table_lock); Please take the unlock out, it is clearly an anti-pattern. And please Cc netdev for networking changes. Thanks.
On Sat, Feb 20, 2021 at 11:12:33AM -0800, Cong Wang wrote: > On Thu, Feb 18, 2021 at 8:22 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > Duplicated logics in all bind variants (autobind, bind-to-path, > > bind-to-abstract) gets taken into a common helper. > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > --- > > net/unix/af_unix.c | 30 +++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > index 41c3303c3357..179b4fe837e6 100644 > > --- a/net/unix/af_unix.c > > +++ b/net/unix/af_unix.c > > @@ -262,6 +262,16 @@ static void __unix_insert_socket(struct hlist_head *list, struct sock *sk) > > sk_add_node(sk, list); > > } > > > > +static void __unix_set_addr(struct sock *sk, struct unix_address *addr, > > + unsigned hash) > > + __releases(&unix_table_lock) > > +{ > > + __unix_remove_socket(sk); > > + smp_store_release(&unix_sk(sk)->addr, addr); > > + __unix_insert_socket(&unix_socket_table[hash], sk); > > + spin_unlock(&unix_table_lock); > > Please take the unlock out, it is clearly an anti-pattern. Why? "Insert into locked and unlock" is fairly common... > And please Cc netdev for networking changes. Sorry about that - I'd ended up emulating git send-email by hand and missed the Cc. Sorted out by now...
On Sat, Feb 20, 2021 at 11:32 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sat, Feb 20, 2021 at 11:12:33AM -0800, Cong Wang wrote: > > On Thu, Feb 18, 2021 at 8:22 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > Duplicated logics in all bind variants (autobind, bind-to-path, > > > bind-to-abstract) gets taken into a common helper. > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > --- > > > net/unix/af_unix.c | 30 +++++++++++++++--------------- > > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > > index 41c3303c3357..179b4fe837e6 100644 > > > --- a/net/unix/af_unix.c > > > +++ b/net/unix/af_unix.c > > > @@ -262,6 +262,16 @@ static void __unix_insert_socket(struct hlist_head *list, struct sock *sk) > > > sk_add_node(sk, list); > > > } > > > > > > +static void __unix_set_addr(struct sock *sk, struct unix_address *addr, > > > + unsigned hash) > > > + __releases(&unix_table_lock) > > > +{ > > > + __unix_remove_socket(sk); > > > + smp_store_release(&unix_sk(sk)->addr, addr); > > > + __unix_insert_socket(&unix_socket_table[hash], sk); > > > + spin_unlock(&unix_table_lock); > > > > Please take the unlock out, it is clearly an anti-pattern. > > Why? "Insert into locked and unlock" is fairly common... Because it does not lock the lock, just compare: lock(); __unix_set_addr(); unlock(); to: lock(); __unix_set_addr(); Clearly the former is more readable and less error-prone. Even if you really want to do unlock, pick a name which explicitly says it, for example, __unix_set_addr_unlock(). Thanks.
On Sat, Feb 20, 2021 at 12:31:49PM -0800, Cong Wang wrote: > Because it does not lock the lock, just compare: > > lock(); > __unix_set_addr(); > unlock(); > > to: > > lock(); > __unix_set_addr(); > > Clearly the former is more readable and less error-prone. Even > if you really want to do unlock, pick a name which explicitly says > it, for example, __unix_set_addr_unlock(). *shrug* If anything, __unix_complete_bind() might make a better name for that, with dropping ->bindlock also pulled in, but TBH I don't have sufficiently strong preferences - might as well leave dropping the lock to caller. I'll post that series to netdev tonight.
On Sat, Feb 20, 2021 at 09:08:56PM +0000, Al Viro wrote: > *shrug* > > If anything, __unix_complete_bind() might make a better name for that, > with dropping ->bindlock also pulled in, but TBH I don't have sufficiently > strong preferences - might as well leave dropping the lock to caller. > > I'll post that series to netdev tonight. Took longer than I hoped... Anyway, here's the current variant; it's 5.11-based, lives in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git misc.af_unix Shortlog: Al Viro (8): af_unix: take address assignment/hash insertion into a new helper unix_bind(): allocate addr earlier unix_bind(): separate BSD and abstract cases unix_bind(): take BSD and abstract address cases into new helpers fold unix_mknod() into unix_bind_bsd() unix_bind_bsd(): move done_path_create() call after dealing with ->bindlock unix_bind_bsd(): unlink if we fail after successful mknod __unix_find_socket_byname(): don't pass hash and type separately Diffstat: net/unix/af_unix.c | 186 +++++++++++++++++++++++++++-------------------------- 1 file changed, 94 insertions(+), 92 deletions(-) The actual fix is in #7/8, the first 6 are massage in preparation to that and #8/8 is a minor followup cleanup. Individual patches in followups. Please, review.
On Mon, Feb 22, 2021 at 07:06:00PM +0000, Al Viro wrote: > On Sat, Feb 20, 2021 at 09:08:56PM +0000, Al Viro wrote: > > > *shrug* > > > > If anything, __unix_complete_bind() might make a better name for that, > > with dropping ->bindlock also pulled in, but TBH I don't have sufficiently > > strong preferences - might as well leave dropping the lock to caller. > > > > I'll post that series to netdev tonight. > > Took longer than I hoped... Anyway, here's the current variant; > it's 5.11-based, lives in > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git misc.af_unix > > Shortlog: > Al Viro (8): > af_unix: take address assignment/hash insertion into a new helper > unix_bind(): allocate addr earlier > unix_bind(): separate BSD and abstract cases > unix_bind(): take BSD and abstract address cases into new helpers > fold unix_mknod() into unix_bind_bsd() > unix_bind_bsd(): move done_path_create() call after dealing with ->bindlock > unix_bind_bsd(): unlink if we fail after successful mknod > __unix_find_socket_byname(): don't pass hash and type separately > > Diffstat: > net/unix/af_unix.c | 186 +++++++++++++++++++++++++++-------------------------- > 1 file changed, 94 insertions(+), 92 deletions(-) > > The actual fix is in #7/8, the first 6 are massage in preparation to that > and #8/8 is a minor followup cleanup. Individual patches in followups. > Please, review. Argh... git send-email is playing silly buggers again ;-/
On Mon, Feb 22, 2021 at 07:12:29PM +0000, Al Viro wrote: > On Mon, Feb 22, 2021 at 07:06:00PM +0000, Al Viro wrote: > > On Sat, Feb 20, 2021 at 09:08:56PM +0000, Al Viro wrote: > > > > > *shrug* > > > > > > If anything, __unix_complete_bind() might make a better name for that, > > > with dropping ->bindlock also pulled in, but TBH I don't have sufficiently > > > strong preferences - might as well leave dropping the lock to caller. > > > > > > I'll post that series to netdev tonight. > > > > Took longer than I hoped... Anyway, here's the current variant; > > it's 5.11-based, lives in > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git misc.af_unix > > > > Shortlog: > > Al Viro (8): > > af_unix: take address assignment/hash insertion into a new helper > > unix_bind(): allocate addr earlier > > unix_bind(): separate BSD and abstract cases > > unix_bind(): take BSD and abstract address cases into new helpers > > fold unix_mknod() into unix_bind_bsd() > > unix_bind_bsd(): move done_path_create() call after dealing with ->bindlock > > unix_bind_bsd(): unlink if we fail after successful mknod > > __unix_find_socket_byname(): don't pass hash and type separately > > > > Diffstat: > > net/unix/af_unix.c | 186 +++++++++++++++++++++++++++-------------------------- > > 1 file changed, 94 insertions(+), 92 deletions(-) > > > > The actual fix is in #7/8, the first 6 are massage in preparation to that > > and #8/8 is a minor followup cleanup. Individual patches in followups. > > Please, review. > > Argh... git send-email is playing silly buggers again ;-/ Cute - looks like having EMAIL in environment confuses the living hell out git-send-email. Oh, well...
On Mon, 22 Feb 2021 19:06:00 +0000 Al Viro wrote: > On Sat, Feb 20, 2021 at 09:08:56PM +0000, Al Viro wrote: > > > *shrug* > > > > If anything, __unix_complete_bind() might make a better name for that, > > with dropping ->bindlock also pulled in, but TBH I don't have sufficiently > > strong preferences - might as well leave dropping the lock to caller. > > > > I'll post that series to netdev tonight. > > Took longer than I hoped... Anyway, here's the current variant; > it's 5.11-based, lives in > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git misc.af_unix > > Shortlog: > Al Viro (8): > af_unix: take address assignment/hash insertion into a new helper > unix_bind(): allocate addr earlier > unix_bind(): separate BSD and abstract cases > unix_bind(): take BSD and abstract address cases into new helpers > fold unix_mknod() into unix_bind_bsd() > unix_bind_bsd(): move done_path_create() call after dealing with ->bindlock > unix_bind_bsd(): unlink if we fail after successful mknod > __unix_find_socket_byname(): don't pass hash and type separately > > Diffstat: > net/unix/af_unix.c | 186 +++++++++++++++++++++++++++-------------------------- > 1 file changed, 94 insertions(+), 92 deletions(-) > > The actual fix is in #7/8, the first 6 are massage in preparation to that > and #8/8 is a minor followup cleanup. Individual patches in followups. Dave is out this week, but this looks good to me. You said "please review" - I'm assuming you'll send these to Linus yourself, so: Acked-by: Jakub Kicinski <kuba@kernel.org>
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 41c3303c3357..179b4fe837e6 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -262,6 +262,16 @@ static void __unix_insert_socket(struct hlist_head *list, struct sock *sk) sk_add_node(sk, list); } +static void __unix_set_addr(struct sock *sk, struct unix_address *addr, + unsigned hash) + __releases(&unix_table_lock) +{ + __unix_remove_socket(sk); + smp_store_release(&unix_sk(sk)->addr, addr); + __unix_insert_socket(&unix_socket_table[hash], sk); + spin_unlock(&unix_table_lock); +} + static inline void unix_remove_socket(struct sock *sk) { spin_lock(&unix_table_lock); @@ -912,10 +922,7 @@ static int unix_autobind(struct socket *sock) } addr->hash ^= sk->sk_type; - __unix_remove_socket(sk); - smp_store_release(&u->addr, addr); - __unix_insert_socket(&unix_socket_table[addr->hash], sk); - spin_unlock(&unix_table_lock); + __unix_set_addr(sk, addr, addr->hash); err = 0; out: mutex_unlock(&u->bindlock); @@ -1016,7 +1023,6 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) int err; unsigned int hash; struct unix_address *addr; - struct hlist_head *list; struct path path = { }; err = -EINVAL; @@ -1068,26 +1074,20 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) hash = d_backing_inode(path.dentry)->i_ino & (UNIX_HASH_SIZE - 1); spin_lock(&unix_table_lock); u->path = path; - list = &unix_socket_table[hash]; } else { spin_lock(&unix_table_lock); err = -EADDRINUSE; if (__unix_find_socket_byname(net, sunaddr, addr_len, sk->sk_type, hash)) { + spin_unlock(&unix_table_lock); unix_release_addr(addr); - goto out_unlock; + goto out_up; } - - list = &unix_socket_table[addr->hash]; + hash = addr->hash; } + __unix_set_addr(sk, addr, hash); err = 0; - __unix_remove_socket(sk); - smp_store_release(&u->addr, addr); - __unix_insert_socket(list, sk); - -out_unlock: - spin_unlock(&unix_table_lock); out_up: mutex_unlock(&u->bindlock); out_put:
Duplicated logics in all bind variants (autobind, bind-to-path, bind-to-abstract) gets taken into a common helper. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- net/unix/af_unix.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)