| Message ID | 20201114103625.323919-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: x25: Correct locking for x25_kill_by_device and x25_kill_by_neigh | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 46 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sat, Nov 14, 2020 at 2:36 AM Xie He <xie.he.0141@gmail.com> wrote: > > This patch adds correct locking for x25_kill_by_device and > x25_kill_by_neigh, and removes the incorrect locking in x25_connect and > x25_disconnect. I see if I do this change, I need to make sure the sock lock is not held when calling x25_remove_socket, to prevent deadlock. Sorry. I'll deal with this issue and resubmit. I also see that in x25_find_listener and __x25_find_socket, when we traverse x25_list, we should probably also hold the sock lock when we read the element of the list, and continue to hold the lock when we find the sock we want.
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index a10487e7574c..50f043f0c1d0 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -208,9 +208,12 @@ static void x25_kill_by_device(struct net_device *dev) write_lock_bh(&x25_list_lock); - sk_for_each(s, &x25_list) + sk_for_each(s, &x25_list) { + bh_lock_sock(s); if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev) x25_disconnect(s, ENETUNREACH, 0, 0); + bh_unlock_sock(s); + } write_unlock_bh(&x25_list_lock); } @@ -826,10 +829,8 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr, rc = 0; out_put_neigh: if (rc && x25->neighbour) { - read_lock_bh(&x25_list_lock); x25_neigh_put(x25->neighbour); x25->neighbour = NULL; - read_unlock_bh(&x25_list_lock); x25->state = X25_STATE_0; } out_put_route: @@ -1773,9 +1774,12 @@ void x25_kill_by_neigh(struct x25_neigh *nb) write_lock_bh(&x25_list_lock); - sk_for_each(s, &x25_list) + sk_for_each(s, &x25_list) { + bh_lock_sock(s); if (x25_sk(s)->neighbour == nb) x25_disconnect(s, ENETUNREACH, 0, 0); + bh_unlock_sock(s); + } write_unlock_bh(&x25_list_lock); diff --git a/net/x25/x25_subr.c b/net/x25/x25_subr.c index 0285aaa1e93c..6c0f94257f7c 100644 --- a/net/x25/x25_subr.c +++ b/net/x25/x25_subr.c @@ -358,10 +358,8 @@ void x25_disconnect(struct sock *sk, int reason, unsigned char cause, sock_set_flag(sk, SOCK_DEAD); } if (x25->neighbour) { - read_lock_bh(&x25_list_lock); x25_neigh_put(x25->neighbour); x25->neighbour = NULL; - read_unlock_bh(&x25_list_lock); } }
When the x25_connect function and the x25_disconnect function decrease the refcnt of "x25->neighbour" (struct x25_neigh) and reset this pointer to NULL, they would hold the x25_list_lock read lock. This is weird, because x25_list_lock is meant to protect x25_list, and neither the refcnt of "struct x25_neigh" nor the "x25->neighbour" pointer is related to x25_list itself. I checked the commit history. The author who added the locking in x25_disconnect didn't explain why in the commit message. I think they probably just copied the code from x25_connect. The author who added the locking in x25_connect did this probably because he wanted to protect the code from racing with x25_kill_by_device. However, I think this is not the correct way to protect from racing between x25_connect and x25_kill_by_device. The correct way should be letting x25_kill_by_device hold the appropriate sock lock instead. For x25_disconnect, holding x25_list_lock not only is incorrect, but also causes deadlock, because x25_disconnect is called by x25_kill_by_device with the x25_list_lock write lock held. For x25_kill_by_neigh, the situation is the same as x25_kill_by_device. This patch adds correct locking for x25_kill_by_device and x25_kill_by_neigh, and removes the incorrect locking in x25_connect and x25_disconnect. Fixes: 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25 disconnect") Fixes: 95d6ebd53c79 ("net/x25: fix use-after-free in x25_device_event()") Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- net/x25/af_x25.c | 12 ++++++++---- net/x25/x25_subr.c | 2 -- 2 files changed, 8 insertions(+), 6 deletions(-)