[PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

[PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

[Nagamani PV]

afiucv_netdev_event() walks iucv_sk_list.head inside a bare
sk_for_each() with no lock held, while concurrent paths can modify
or destroy sockets in that list:

BUG-1  Use-After-Free
  iucv_sock_kill() calls iucv_sock_unlink() under write_lock_bh
  followed by sock_put() which may free the sk.  If the notifier
  is mid-traversal when sock_put() runs, the stale sk pointer
  dereference at iucv = iucv_sk(sk) is a use-after-free.
  KASAN reports: slab-use-after-free in afiucv_netdev_event.
  Confirmed with KASAN on IBM Z.

BUG-2  Locking correctness: iucv_send_ctrl() without lock_sock()
  iucv_send_ctrl() is called without holding lock_sock(sk).  The
  notifier reads iucv->hs_dev without the socket lock, while
  iucv_sock_close() writes iucv->hs_dev = NULL under lock_sock().
  iucv_send_ctrl() also reads and writes sk->sk_shutdown as bare
  accesses, racing with iucv_sock_close() which holds lock_sock()
  for the same fields.  KCSAN reports both races.
  Confirmed with KCSAN on IBM Z:

    write to iucv->hs_dev of 8 bytes by task N on cpu M:
      iucv_sock_close+0x196 [af_iucv]    (under lock_sock)

    read to iucv->hs_dev by task N on cpu M:
      afiucv_netdev_event [af_iucv]      (no lock held)

BUG-3  sk_state data race
  sk->sk_state is written without lock_sock() in the notifier, racing
  with iucv_sock_close() writing the same field under lock_sock().
  KCSAN reports: data-race in afiucv_netdev_event / iucv_sock_close.
  Confirmed with KCSAN on IBM Z:

    write to sk->sk_state by task N on cpu M:
      iucv_sock_close+0x196 [af_iucv]   (under lock_sock)

    read to sk->sk_state by task N on cpu M:
      afiucv_netdev_event+0xa6 [af_iucv] (no lock held)

Fix with a two-pass algorithm:

  Pass 1 (read_lock_bh): walk iucv_sk_list, sock_hold() each
    matching socket, collect into a local list.  The read lock
    prevents concurrent write_lock_bh in iucv_sock_link/unlink
    from modifying the list while we take references.

  Pass 2 (lock_sock per socket): for each collected socket,
    acquire lock_sock to serialise against iucv_sock_close(),
    check sk_state under the lock, call iucv_send_ctrl() and
    update sk_state safely, then release_sock() + sock_put().

This eliminates all three races:
  - BUG-1: read_lock_bh prevents sk from being unlinked and freed
    while we hold a reference to it.
  - BUG-2: iucv_send_ctrl() is now called under lock_sock(), not
    racing with concurrent socket close.
  - BUG-3: sk_state is read and written under lock_sock(),
    serialising against iucv_sock_close().

Pass 1 reads iucv_sk(sk)->hs_dev under read_lock_bh to filter
sockets belonging to the affected device.  iucv_sock_close() writes
hs_dev = NULL under lock_sock(), which is orthogonal to read_lock_bh.
Use READ_ONCE() for the Pass 1 read and WRITE_ONCE() for the
iucv_sock_close() write to document the intentional concurrent access
and suppress KCSAN false positives.  The read is safe: read_lock_bh
prevents the socket from being freed; if hs_dev is concurrently
cleared to NULL it will not match event_dev (a valid pointer) so the
socket is correctly skipped.

Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
Reported-by: Bryam Vargas <hexlabsecurity@proton.me>
Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
---
Changes since v1 (2-line read_lock_bh fix, posted 2026-08-03):
  - Redesign as two-pass algorithm to fix all races in one patch:
    Pass 1: read_lock_bh + sock_hold() to safely collect matching
    sockets; Pass 2: lock_sock() per socket to act on sk_state,
    call iucv_send_ctrl() and update sk_state under the lock.
  - Fixes Sashiko Finding 1 (New/High): sleep-in-atomic - V1 called
    iucv_send_ctrl() inside read_lock_bh(); Pass 2 runs after
    read_unlock_bh() so GFP_KERNEL allocation is safe.
  - Fixes Sashiko Finding 2 (Pre-existing/High): lockless manipulation
    of sk_state, sk_shutdown, sk_socket - all now under lock_sock().
  - Add WRITE_ONCE(iucv->hs_dev, NULL) in iucv_sock_close() and
    READ_ONCE(iucv_sk(sk)->hs_dev) in Pass 1 to document intentional
    concurrent access across orthogonal locks and suppress KCSAN.
  - All three bugs confirmed with KASAN + KCSAN on IBM Z with
    before/after TAP results.
  - Retarget from net-next to net (Fixes: + Cc: stable).
  - Update subject from "fix UAF" to "fix races" to reflect full scope.

Bryam Vargas: your RFC identified the lockless socket manipulation in
afiucv_netdev_event() as part of your 17-context analysis.  I have
included Reported-by for that attribution.  Please let me know if you
are happy with this, or prefer a different tag.

 net/iucv/af_iucv.c | 57 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..b197f9a254a7 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -320,6 +320,12 @@ static void iucv_sock_unlink(struct iucv_sock_list *l, struct sock *sk)
 	write_unlock_bh(&l->lock);
 }
 
+/* Used by afiucv_netdev_event() two-pass algorithm */
+struct iucv_netdev_todo {
+	struct list_head list;
+	struct sock *sk;
+};
+
 /* Kill socket (only if zapped and orphaned) */
 static void iucv_sock_kill(struct sock *sk)
 {
@@ -445,7 +451,7 @@ static void iucv_sock_close(struct sock *sk)
 
 	if (iucv->hs_dev) {
 		dev_put(iucv->hs_dev);
-		iucv->hs_dev = NULL;
+		WRITE_ONCE(iucv->hs_dev, NULL);
 		sk->sk_bound_dev_if = 0;
 	}
 
@@ -2207,21 +2213,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
 			       unsigned long event, void *ptr)
 {
 	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
+	struct iucv_netdev_todo *entry, *tmp;
+	LIST_HEAD(todo);
 	struct sock *sk;
-	struct iucv_sock *iucv;
 
 	switch (event) {
 	case NETDEV_REBOOT:
 	case NETDEV_GOING_DOWN:
+		/*
+		 * Pass 1: collect matching sockets under read_lock_bh.
+		 *
+		 * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
+		 * write_lock_bh in iucv_sock_link/unlink, so sk cannot
+		 * be removed from the list or freed while we walk it.
+		 * sock_hold() pins the sk so it survives after we drop
+		 * the lock.
+		 *
+		 * iucv_sock_close() writes hs_dev = NULL under lock_sock,
+		 * which is orthogonal to read_lock_bh.  READ_ONCE() documents
+		 * the intentional concurrent access: if hs_dev is being
+		 * cleared to NULL it will not equal event_dev (a valid
+		 * pointer) so the socket is correctly skipped.
+		 */
+		read_lock_bh(&iucv_sk_list.lock);
 		sk_for_each(sk, &iucv_sk_list.head) {
-			iucv = iucv_sk(sk);
-			if ((iucv->hs_dev == event_dev) &&
-			    (sk->sk_state == IUCV_CONNECTED)) {
+			if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
+				continue;
+			entry = kmalloc_obj(*entry, GFP_ATOMIC);
+			if (!entry)
+				continue;
+			sock_hold(sk);
+			entry->sk = sk;
+			list_add_tail(&entry->list, &todo);
+		}
+		read_unlock_bh(&iucv_sk_list.lock);
+		/*
+		 * Pass 2: act on each socket under lock_sock.
+		 *
+		 * lock_sock() serialises against iucv_sock_close() and
+		 * sock_orphan(), so sk_state and sk_socket are stable.
+		 * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
+		 * which requires non-atomic context -- satisfied here because
+		 * we are no longer holding read_lock_bh.
+		 */
+		list_for_each_entry_safe(entry, tmp, &todo, list) {
+			sk = entry->sk;
+			lock_sock(sk);
+			if (sk->sk_state == IUCV_CONNECTED) {
 				if (event == NETDEV_GOING_DOWN)
 					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
 				sk->sk_state = IUCV_DISCONN;
 				sk->sk_state_change(sk);
 			}
+			release_sock(sk);
+			sock_put(sk);
+			list_del(&entry->list);
+			kfree(entry);
 		}
 		break;
 	case NETDEV_DOWN:
-- 
2.47.1

Re: [PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

[Alexandra Winter]

On 17.09.26 09:17, Nagamani PV wrote:
> afiucv_netdev_event() walks iucv_sk_list.head inside a bare
> sk_for_each() with no lock held, while concurrent paths can modify
> or destroy sockets in that list:
> 
> BUG-1  Use-After-Free
>   iucv_sock_kill() calls iucv_sock_unlink() under write_lock_bh
>   followed by sock_put() which may free the sk.  If the notifier
>   is mid-traversal when sock_put() runs, the stale sk pointer
>   dereference at iucv = iucv_sk(sk) is a use-after-free.
>   KASAN reports: slab-use-after-free in afiucv_netdev_event.
>   Confirmed with KASAN on IBM Z.
> 
> BUG-2  Locking correctness: iucv_send_ctrl() without lock_sock()
>   iucv_send_ctrl() is called without holding lock_sock(sk).  The
>   notifier reads iucv->hs_dev without the socket lock, while
>   iucv_sock_close() writes iucv->hs_dev = NULL under lock_sock().
>   iucv_send_ctrl() also reads and writes sk->sk_shutdown as bare
>   accesses, racing with iucv_sock_close() which holds lock_sock()
>   for the same fields.  KCSAN reports both races.
>   Confirmed with KCSAN on IBM Z:
> 
>     write to iucv->hs_dev of 8 bytes by task N on cpu M:
>       iucv_sock_close+0x196 [af_iucv]    (under lock_sock)
> 
>     read to iucv->hs_dev by task N on cpu M:
>       afiucv_netdev_event [af_iucv]      (no lock held)
> 
> BUG-3  sk_state data race
>   sk->sk_state is written without lock_sock() in the notifier, racing
>   with iucv_sock_close() writing the same field under lock_sock().
>   KCSAN reports: data-race in afiucv_netdev_event / iucv_sock_close.
>   Confirmed with KCSAN on IBM Z:
> 
>     write to sk->sk_state by task N on cpu M:
>       iucv_sock_close+0x196 [af_iucv]   (under lock_sock)
> 
>     read to sk->sk_state by task N on cpu M:
>       afiucv_netdev_event+0xa6 [af_iucv] (no lock held)
> 
> Fix with a two-pass algorithm:
> 
>   Pass 1 (read_lock_bh): walk iucv_sk_list, sock_hold() each
>     matching socket, collect into a local list.  The read lock
>     prevents concurrent write_lock_bh in iucv_sock_link/unlink
>     from modifying the list while we take references.
> 
>   Pass 2 (lock_sock per socket): for each collected socket,
>     acquire lock_sock to serialise against iucv_sock_close(),
>     check sk_state under the lock, call iucv_send_ctrl() and
>     update sk_state safely, then release_sock() + sock_put().
> 
> This eliminates all three races:
>   - BUG-1: read_lock_bh prevents sk from being unlinked and freed
>     while we hold a reference to it.
>   - BUG-2: iucv_send_ctrl() is now called under lock_sock(), not
>     racing with concurrent socket close.
>   - BUG-3: sk_state is read and written under lock_sock(),
>     serialising against iucv_sock_close().
> 
> Pass 1 reads iucv_sk(sk)->hs_dev under read_lock_bh to filter
> sockets belonging to the affected device.  iucv_sock_close() writes
> hs_dev = NULL under lock_sock(), which is orthogonal to read_lock_bh.
> Use READ_ONCE() for the Pass 1 read and WRITE_ONCE() for the
> iucv_sock_close() write to document the intentional concurrent access
> and suppress KCSAN false positives.  The read is safe: read_lock_bh
> prevents the socket from being freed; if hs_dev is concurrently
> cleared to NULL it will not match event_dev (a valid pointer) so the
> socket is correctly skipped.
> 
> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
> Reported-by: Bryam Vargas <hexlabsecurity@proton.me>
> Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
> ---
> Changes since v1 (2-line read_lock_bh fix, posted 2026-08-03):
>   - Redesign as two-pass algorithm to fix all races in one patch:
>     Pass 1: read_lock_bh + sock_hold() to safely collect matching
>     sockets; Pass 2: lock_sock() per socket to act on sk_state,
>     call iucv_send_ctrl() and update sk_state under the lock.
>   - Fixes Sashiko Finding 1 (New/High): sleep-in-atomic - V1 called
>     iucv_send_ctrl() inside read_lock_bh(); Pass 2 runs after
>     read_unlock_bh() so GFP_KERNEL allocation is safe.
>   - Fixes Sashiko Finding 2 (Pre-existing/High): lockless manipulation
>     of sk_state, sk_shutdown, sk_socket - all now under lock_sock().
>   - Add WRITE_ONCE(iucv->hs_dev, NULL) in iucv_sock_close() and
>     READ_ONCE(iucv_sk(sk)->hs_dev) in Pass 1 to document intentional
>     concurrent access across orthogonal locks and suppress KCSAN.
>   - All three bugs confirmed with KASAN + KCSAN on IBM Z with
>     before/after TAP results.
>   - Retarget from net-next to net (Fixes: + Cc: stable).
>   - Update subject from "fix UAF" to "fix races" to reflect full scope.
> 
> Bryam Vargas: your RFC identified the lockless socket manipulation in
> afiucv_netdev_event() as part of your 17-context analysis.  I have
> included Reported-by for that attribution.  Please let me know if you
> are happy with this, or prefer a different tag.


Nagamani,
I have told you before, that I want us to work on a correct usage of
of lock_sock() and  bh_lock_sock() in af_iucv.c (stage 1 and 2 in Bryam's plan [1])
 before fixing the callers (stage 3).

The races you are referring to belong to the group that needs fixing.

afiucv_netdev_event() needs a lock, as Bryam mentioned. The 2 pass approach looks
good to me in general. But without a proper implemenation of the lock_sock mechanism,
it is far from complete.

Please hold it until Stage 1 (for TRANS_HIPER) is implemented.


> 
>  net/iucv/af_iucv.c | 57 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..b197f9a254a7 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -320,6 +320,12 @@ static void iucv_sock_unlink(struct iucv_sock_list *l, struct sock *sk)
>  	write_unlock_bh(&l->lock);
>  }
>  
> +/* Used by afiucv_netdev_event() two-pass algorithm */
> +struct iucv_netdev_todo {
> +	struct list_head list;
> +	struct sock *sk;
> +};
> +
>  /* Kill socket (only if zapped and orphaned) */
>  static void iucv_sock_kill(struct sock *sk)
>  {
> @@ -445,7 +451,7 @@ static void iucv_sock_close(struct sock *sk)
>  
>  	if (iucv->hs_dev) {
>  		dev_put(iucv->hs_dev);
> -		iucv->hs_dev = NULL;
> +		WRITE_ONCE(iucv->hs_dev, NULL);
>  		sk->sk_bound_dev_if = 0;
>  	}
>  
> @@ -2207,21 +2213,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  			       unsigned long event, void *ptr)
>  {
>  	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +	struct iucv_netdev_todo *entry, *tmp;
> +	LIST_HEAD(todo);
>  	struct sock *sk;
> -	struct iucv_sock *iucv;
>  
>  	switch (event) {
>  	case NETDEV_REBOOT:
>  	case NETDEV_GOING_DOWN:
> +		/*
> +		 * Pass 1: collect matching sockets under read_lock_bh.
> +		 *
> +		 * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
> +		 * write_lock_bh in iucv_sock_link/unlink, so sk cannot
> +		 * be removed from the list or freed while we walk it.
> +		 * sock_hold() pins the sk so it survives after we drop
> +		 * the lock.
> +		 *
> +		 * iucv_sock_close() writes hs_dev = NULL under lock_sock,
> +		 * which is orthogonal to read_lock_bh.  READ_ONCE() documents
> +		 * the intentional concurrent access: if hs_dev is being
> +		 * cleared to NULL it will not equal event_dev (a valid
> +		 * pointer) so the socket is correctly skipped.
> +		 */
> +		read_lock_bh(&iucv_sk_list.lock);
>  		sk_for_each(sk, &iucv_sk_list.head) {
> -			iucv = iucv_sk(sk);
> -			if ((iucv->hs_dev == event_dev) &&
> -			    (sk->sk_state == IUCV_CONNECTED)) {
> +			if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
> +				continue;
> +			entry = kmalloc_obj(*entry, GFP_ATOMIC);
> +			if (!entry)
> +				continue;
> +			sock_hold(sk);
> +			entry->sk = sk;
> +			list_add_tail(&entry->list, &todo);
> +		}
> +		read_unlock_bh(&iucv_sk_list.lock);
> +		/*
> +		 * Pass 2: act on each socket under lock_sock.
> +		 *
> +		 * lock_sock() serialises against iucv_sock_close() and
> +		 * sock_orphan(), so sk_state and sk_socket are stable.
> +		 * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
> +		 * which requires non-atomic context -- satisfied here because
> +		 * we are no longer holding read_lock_bh.
> +		 */
> +		list_for_each_entry_safe(entry, tmp, &todo, list) {
> +			sk = entry->sk;
> +			lock_sock(sk);
> +			if (sk->sk_state == IUCV_CONNECTED) {
>  				if (event == NETDEV_GOING_DOWN)
>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
>  				sk->sk_state = IUCV_DISCONN;
>  				sk->sk_state_change(sk);
>  			}
> +			release_sock(sk);
> +			sock_put(sk);
> +			list_del(&entry->list);
> +			kfree(entry);
>  		}
>  		break;
>  	case NETDEV_DOWN:

Re: [PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

[Nagamani PV]

On 17/09/26 7:31 PM, Alexandra Winter wrote:
> 
> 
> On 17.09.26 09:17, Nagamani PV wrote:
>> afiucv_netdev_event() walks iucv_sk_list.head inside a bare
>> sk_for_each() with no lock held, while concurrent paths can modify
>> or destroy sockets in that list:
>>
>> BUG-1  Use-After-Free
>>   iucv_sock_kill() calls iucv_sock_unlink() under write_lock_bh
>>   followed by sock_put() which may free the sk.  If the notifier
>>   is mid-traversal when sock_put() runs, the stale sk pointer
>>   dereference at iucv = iucv_sk(sk) is a use-after-free.
>>   KASAN reports: slab-use-after-free in afiucv_netdev_event.
>>   Confirmed with KASAN on IBM Z.
>>
>> BUG-2  Locking correctness: iucv_send_ctrl() without lock_sock()
>>   iucv_send_ctrl() is called without holding lock_sock(sk).  The
>>   notifier reads iucv->hs_dev without the socket lock, while
>>   iucv_sock_close() writes iucv->hs_dev = NULL under lock_sock().
>>   iucv_send_ctrl() also reads and writes sk->sk_shutdown as bare
>>   accesses, racing with iucv_sock_close() which holds lock_sock()
>>   for the same fields.  KCSAN reports both races.
>>   Confirmed with KCSAN on IBM Z:
>>
>>     write to iucv->hs_dev of 8 bytes by task N on cpu M:
>>       iucv_sock_close+0x196 [af_iucv]    (under lock_sock)
>>
>>     read to iucv->hs_dev by task N on cpu M:
>>       afiucv_netdev_event [af_iucv]      (no lock held)
>>
>> BUG-3  sk_state data race
>>   sk->sk_state is written without lock_sock() in the notifier, racing
>>   with iucv_sock_close() writing the same field under lock_sock().
>>   KCSAN reports: data-race in afiucv_netdev_event / iucv_sock_close.
>>   Confirmed with KCSAN on IBM Z:
>>
>>     write to sk->sk_state by task N on cpu M:
>>       iucv_sock_close+0x196 [af_iucv]   (under lock_sock)
>>
>>     read to sk->sk_state by task N on cpu M:
>>       afiucv_netdev_event+0xa6 [af_iucv] (no lock held)
>>
>> Fix with a two-pass algorithm:
>>
>>   Pass 1 (read_lock_bh): walk iucv_sk_list, sock_hold() each
>>     matching socket, collect into a local list.  The read lock
>>     prevents concurrent write_lock_bh in iucv_sock_link/unlink
>>     from modifying the list while we take references.
>>
>>   Pass 2 (lock_sock per socket): for each collected socket,
>>     acquire lock_sock to serialise against iucv_sock_close(),
>>     check sk_state under the lock, call iucv_send_ctrl() and
>>     update sk_state safely, then release_sock() + sock_put().
>>
>> This eliminates all three races:
>>   - BUG-1: read_lock_bh prevents sk from being unlinked and freed
>>     while we hold a reference to it.
>>   - BUG-2: iucv_send_ctrl() is now called under lock_sock(), not
>>     racing with concurrent socket close.
>>   - BUG-3: sk_state is read and written under lock_sock(),
>>     serialising against iucv_sock_close().
>>
>> Pass 1 reads iucv_sk(sk)->hs_dev under read_lock_bh to filter
>> sockets belonging to the affected device.  iucv_sock_close() writes
>> hs_dev = NULL under lock_sock(), which is orthogonal to read_lock_bh.
>> Use READ_ONCE() for the Pass 1 read and WRITE_ONCE() for the
>> iucv_sock_close() write to document the intentional concurrent access
>> and suppress KCSAN false positives.  The read is safe: read_lock_bh
>> prevents the socket from being freed; if hs_dev is concurrently
>> cleared to NULL it will not match event_dev (a valid pointer) so the
>> socket is correctly skipped.
>>
>> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
>> Reported-by: Bryam Vargas <hexlabsecurity@proton.me>
>> Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
>> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
>> ---
>> Changes since v1 (2-line read_lock_bh fix, posted 2026-08-03):
>>   - Redesign as two-pass algorithm to fix all races in one patch:
>>     Pass 1: read_lock_bh + sock_hold() to safely collect matching
>>     sockets; Pass 2: lock_sock() per socket to act on sk_state,
>>     call iucv_send_ctrl() and update sk_state under the lock.
>>   - Fixes Sashiko Finding 1 (New/High): sleep-in-atomic - V1 called
>>     iucv_send_ctrl() inside read_lock_bh(); Pass 2 runs after
>>     read_unlock_bh() so GFP_KERNEL allocation is safe.
>>   - Fixes Sashiko Finding 2 (Pre-existing/High): lockless manipulation
>>     of sk_state, sk_shutdown, sk_socket - all now under lock_sock().
>>   - Add WRITE_ONCE(iucv->hs_dev, NULL) in iucv_sock_close() and
>>     READ_ONCE(iucv_sk(sk)->hs_dev) in Pass 1 to document intentional
>>     concurrent access across orthogonal locks and suppress KCSAN.
>>   - All three bugs confirmed with KASAN + KCSAN on IBM Z with
>>     before/after TAP results.
>>   - Retarget from net-next to net (Fixes: + Cc: stable).
>>   - Update subject from "fix UAF" to "fix races" to reflect full scope.
>>
>> Bryam Vargas: your RFC identified the lockless socket manipulation in
>> afiucv_netdev_event() as part of your 17-context analysis.  I have
>> included Reported-by for that attribution.  Please let me know if you
>> are happy with this, or prefer a different tag.
> 
> 
> Nagamani,
> I have told you before, that I want us to work on a correct usage of
> of lock_sock() and  bh_lock_sock() in af_iucv.c (stage 1 and 2 in Bryam's plan [1])
>  before fixing the callers (stage 3).
> 
> The races you are referring to belong to the group that needs fixing.
> 
> afiucv_netdev_event() needs a lock, as Bryam mentioned. The 2 pass approach looks
> good to me in general. But without a proper implemenation of the lock_sock mechanism,
> it is far from complete.
> 
> Please hold it until Stage 1 (for TRANS_HIPER) is implemented.
> 

I'll hold the afiucv_netdev_event() fix until the Stage 1 locking
rework for TRANS_HIPER is implemented. We can then validate the fix
together with the rework.

> 
>>
>>  net/iucv/af_iucv.c | 57 ++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> index ea047bab65e7..b197f9a254a7 100644
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
>> @@ -320,6 +320,12 @@ static void iucv_sock_unlink(struct iucv_sock_list *l, struct sock *sk)
>>  	write_unlock_bh(&l->lock);
>>  }
>>  
>> +/* Used by afiucv_netdev_event() two-pass algorithm */
>> +struct iucv_netdev_todo {
>> +	struct list_head list;
>> +	struct sock *sk;
>> +};
>> +
>>  /* Kill socket (only if zapped and orphaned) */
>>  static void iucv_sock_kill(struct sock *sk)
>>  {
>> @@ -445,7 +451,7 @@ static void iucv_sock_close(struct sock *sk)
>>  
>>  	if (iucv->hs_dev) {
>>  		dev_put(iucv->hs_dev);
>> -		iucv->hs_dev = NULL;
>> +		WRITE_ONCE(iucv->hs_dev, NULL);
>>  		sk->sk_bound_dev_if = 0;
>>  	}
>>  
>> @@ -2207,21 +2213,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
>>  			       unsigned long event, void *ptr)
>>  {
>>  	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
>> +	struct iucv_netdev_todo *entry, *tmp;
>> +	LIST_HEAD(todo);
>>  	struct sock *sk;
>> -	struct iucv_sock *iucv;
>>  
>>  	switch (event) {
>>  	case NETDEV_REBOOT:
>>  	case NETDEV_GOING_DOWN:
>> +		/*
>> +		 * Pass 1: collect matching sockets under read_lock_bh.
>> +		 *
>> +		 * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
>> +		 * write_lock_bh in iucv_sock_link/unlink, so sk cannot
>> +		 * be removed from the list or freed while we walk it.
>> +		 * sock_hold() pins the sk so it survives after we drop
>> +		 * the lock.
>> +		 *
>> +		 * iucv_sock_close() writes hs_dev = NULL under lock_sock,
>> +		 * which is orthogonal to read_lock_bh.  READ_ONCE() documents
>> +		 * the intentional concurrent access: if hs_dev is being
>> +		 * cleared to NULL it will not equal event_dev (a valid
>> +		 * pointer) so the socket is correctly skipped.
>> +		 */
>> +		read_lock_bh(&iucv_sk_list.lock);
>>  		sk_for_each(sk, &iucv_sk_list.head) {
>> -			iucv = iucv_sk(sk);
>> -			if ((iucv->hs_dev == event_dev) &&
>> -			    (sk->sk_state == IUCV_CONNECTED)) {
>> +			if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
>> +				continue;
>> +			entry = kmalloc_obj(*entry, GFP_ATOMIC);
>> +			if (!entry)
>> +				continue;
>> +			sock_hold(sk);
>> +			entry->sk = sk;
>> +			list_add_tail(&entry->list, &todo);
>> +		}
>> +		read_unlock_bh(&iucv_sk_list.lock);
>> +		/*
>> +		 * Pass 2: act on each socket under lock_sock.
>> +		 *
>> +		 * lock_sock() serialises against iucv_sock_close() and
>> +		 * sock_orphan(), so sk_state and sk_socket are stable.
>> +		 * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
>> +		 * which requires non-atomic context -- satisfied here because
>> +		 * we are no longer holding read_lock_bh.
>> +		 */
>> +		list_for_each_entry_safe(entry, tmp, &todo, list) {
>> +			sk = entry->sk;
>> +			lock_sock(sk);
>> +			if (sk->sk_state == IUCV_CONNECTED) {
>>  				if (event == NETDEV_GOING_DOWN)
>>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
>>  				sk->sk_state = IUCV_DISCONN;
>>  				sk->sk_state_change(sk);
>>  			}
>> +			release_sock(sk);
>> +			sock_put(sk);
>> +			list_del(&entry->list);
>> +			kfree(entry);
>>  		}
>>  		break;
>>  	case NETDEV_DOWN:
> 

Re: [PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 5 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] afiucv_netdev_event() Pass 2 calls lock_sock(sk) for every
  collected socket while the netdevice notifier chain is executing with…
- [Medium] Pass 1 of afiucv_netdev_event() now needs a GFP_ATOMIC
  allocation per affected socket (net/iucv/af_iucv.c:2252).
- [Medium] Pass 2 of afiucv_netdev_event() checks only `sk->sk_state ==
  IUCV_CONNECTED` and then calls iucv_send_ctrl(), which dereferences…
- [Medium] The commit claims BUG-3 ("sk_state data race") is eliminated
  by taking lock_sock() in the notifier, but af_iucv's softirq…
- [Medium] The patch documents (and annotates with READ_ONCE/WRITE_ONCE)
  an invariant that iucv->hs_dev is only concurrently written by…
- [Medium] The new Pass-2 comment in afiucv_netdev_event()
  (net/iucv/af_iucv.c:2263-2264) states that "lock_sock() serialises…
- [Low] The patch introduces a marked-access contract for
  iucv_sock->hs_dev — WRITE_ONCE(iucv->hs_dev, NULL) in…

--

net/iucv: fix races in afiucv_netdev_event()

This reworks afiucv_netdev_event() into a two-pass algorithm: pass one walks
iucv_sk_list under read_lock_bh() and collects matching sockets with
sock_hold() into a local list, pass two takes lock_sock() per socket to send
the FIN and update sk_state.  It also converts the hs_dev clearing in
iucv_sock_close() to WRITE_ONCE() and the notifier read to READ_ONCE().

Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af1..15f5efb405abd 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -448,7 +454,7 @@ static void iucv_sock_close(struct sock *sk)
>  
>  	if (iucv->hs_dev) {
>  		dev_put(iucv->hs_dev);
> -		iucv->hs_dev = NULL;
> +		WRITE_ONCE(iucv->hs_dev, NULL);

[Severity: Low]
Should the other lockless hs_dev accesses be marked as well?  Two of the
four sites that touch hs_dev without the socket lock are left as plain
accesses by this patch:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
	read_lock(&iucv_sk_list.lock);
	sk_for_each(sk, &iucv_sk_list.head) {
		if (iucv_sk(sk)->hs_dev != dev)
			continue;
	...
}

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
	...
	niucv->hs_dev = iucv->hs_dev;
	dev_hold(niucv->hs_dev);
	...
}

With the plain read in afiucv_hs_rcv() still present, is the KCSAN report
the changelog says this suppresses (marked write in iucv_sock_close() versus
unmarked read) still reachable?  And the new READ_ONCE() in pass one is
paired against the unmarked store in afiucv_hs_callback_syn().

>  		sk->sk_bound_dev_if = 0;
>  	}
>  
> @@ -2217,21 +2223,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  			       unsigned long event, void *ptr)
>  {
>  	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +	struct iucv_netdev_todo *entry, *tmp;
> +	LIST_HEAD(todo);
>  	struct sock *sk;
> -	struct iucv_sock *iucv;
>  
>  	switch (event) {
>  	case NETDEV_REBOOT:
>  	case NETDEV_GOING_DOWN:
> +		/*
> +		 * Pass 1: collect matching sockets under read_lock_bh.
> +		 *
> +		 * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
> +		 * write_lock_bh in iucv_sock_link/unlink, so sk cannot
> +		 * be removed from the list or freed while we walk it.
> +		 * sock_hold() pins the sk so it survives after we drop
> +		 * the lock.
> +		 *
> +		 * iucv_sock_close() writes hs_dev = NULL under lock_sock,
> +		 * which is orthogonal to read_lock_bh.  READ_ONCE() documents
> +		 * the intentional concurrent access: if hs_dev is being
> +		 * cleared to NULL it will not equal event_dev (a valid
> +		 * pointer) so the socket is correctly skipped.
> +		 */

[Severity: Medium]
Is iucv_sock_close() really the only concurrent writer of hs_dev?
afiucv_hs_callback_syn() also writes it from softirq context:

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
	nsk = iucv_sock_alloc(NULL, sk->sk_protocol, GFP_ATOMIC, 0);
	bh_lock_sock(sk);
	...
	niucv->hs_dev = iucv->hs_dev;
	dev_hold(niucv->hs_dev);
	...
	nsk->sk_state = IUCV_CONNECTED;
}

iucv_sock_alloc() ends with iucv_sock_link(&iucv_sk_list, sk), so the child
is already visible in iucv_sk_list before hs_dev and sk_state are written,
and the only lock held here is the parent's bh_lock_sock().

That means read_lock_bh() in pass one does not exclude this writer, and
lock_sock(nsk) in pass two does not exclude the softirq that is still
finishing the child's setup.  Can the notifier then send a FIN on a child
socket in parallel with its SYN-ACK handling, or skip a child that turns
IUCV_CONNECTED on the device that is going down?

Could the comment be extended to cover this writer?

> +		read_lock_bh(&iucv_sk_list.lock);
>  		sk_for_each(sk, &iucv_sk_list.head) {
> -			iucv = iucv_sk(sk);
> -			if ((iucv->hs_dev == event_dev) &&
> -			    (sk->sk_state == IUCV_CONNECTED)) {
> +			if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
> +				continue;
> +			entry = kmalloc_obj(*entry, GFP_ATOMIC);
> +			if (!entry)
> +				continue;

[Severity: Medium]
What happens to a matching socket when this GFP_ATOMIC allocation fails?

The continue skips sock_hold() and the todo entry, so pass two never runs
for that socket: no FIN is sent through iucv_send_ctrl() on
NETDEV_GOING_DOWN, sk_state stays IUCV_CONNECTED with iucv->hs_dev still
pointing at the device going down, and sk_state_change() is never called.

Waiters depend on that transition:

net/iucv/af_iucv.c:iucv_sock_recvmsg() {
	if ((sk->sk_state == IUCV_DISCONN) &&
	    skb_queue_empty(&iucv->backlog_skb_q) && ...)
		return 0;
}

net/iucv/af_iucv.c:iucv_sock_poll() {
	if (sk->sk_state == IUCV_DISCONN)
		mask |= EPOLLIN;
}

There is no retry either, since the later notifications do nothing:

	case NETDEV_DOWN:
	case NETDEV_UNREGISTER:
	default:
		break;

The old loop needed no memory at all and could not skip a matching socket.
Could this be done without a per-socket allocation, for example sock_hold()
one socket, drop the list lock, act on it and restart the walk, or with a
list node embedded in struct iucv_sock?

The changelog describes the change purely as a race fix; should it mention
that a socket can now be skipped entirely under memory pressure?

> +			sock_hold(sk);
> +			entry->sk = sk;
> +			list_add_tail(&entry->list, &todo);
> +		}
> +		read_unlock_bh(&iucv_sk_list.lock);
> +		/*
> +		 * Pass 2: act on each socket under lock_sock.
> +		 *
> +		 * lock_sock() serialises against iucv_sock_close() and
> +		 * sock_orphan(), so sk_state and sk_socket are stable.

[Severity: Medium]
Does lock_sock() serialise against sock_orphan() here?

net/iucv/af_iucv.c:iucv_sock_release() {
	iucv_sock_close(sk);

	sock_orphan(sk);
	iucv_sock_kill(sk);
}

iucv_sock_close() takes and releases the socket lock internally, so
sock_orphan() runs with the socket lock not held, and sock_orphan() itself
uses a different lock:

include/net/sock.h:sock_orphan() {
	write_lock_bh(&sk->sk_callback_lock);
	sock_set_flag(sk, SOCK_DEAD);
	sk_set_socket(sk, NULL);
	sk->sk_wq = NULL;
	write_unlock_bh(&sk->sk_callback_lock);
}

The sk_socket claim matters because iucv_send_ctrl() ->
sock_alloc_send_pskb() dereferences sk->sk_socket->flags on the no-space
path.  Similarly, sk_state is written by the softirq callbacks under
bh_lock_sock() only, and there is no sock_owned_by_user() check anywhere in
this file.  Could the comment be narrowed to what lock_sock() actually
excludes?

> +		 * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
> +		 * which requires non-atomic context -- satisfied here because
> +		 * we are no longer holding read_lock_bh.
> +		 */
> +		list_for_each_entry_safe(entry, tmp, &todo, list) {
> +			sk = entry->sk;
> +			lock_sock(sk);

[Severity: High]
Can this stall the netdevice notifier chain while rtnl_lock is held?

afiucv_netdev_event() runs from the notifier chain with rtnl_lock held
(NETDEV_GOING_DOWN via __dev_close_many(), NETDEV_REBOOT via the reboot
notifier), and lock_sock() here waits for whoever currently owns the socket.

An AF_IUCV sender owns the socket lock across the whole send:

net/iucv/af_iucv.c:iucv_sock_sendmsg() {
	lock_sock(sk);
	...
	skb = sock_alloc_send_pskb(sk, headroom + linear, len - linear,
				   noblock, &err, 0);
	...
}

and the wait for send space does not drop that lock:

net/core/sock.c:sock_wait_for_wmem() {
	/* It is almost wait_for_tcp_memory minus release_sock/lock_sock. */
	...
	timeo = schedule_timeout(timeo);
	...
}

sk_sndtimeo starts at IUCV_CONN_TIMEOUT (40 * HZ) for AF_IUCV, and
setsockopt(SO_SNDTIMEO, {0,0}) makes it MAX_SCHEDULE_TIMEOUT:

net/core/sock.c:sock_set_timeout() {
	val = MAX_SCHEDULE_TIMEOUT;
	if ((tv.tv_sec || tv.tv_usec) &&
	    (tv.tv_sec < (MAX_SCHEDULE_TIMEOUT / HZ - 1)))
		val = tv.tv_sec * HZ + ...;
}

The queued TX skbs that hold sk_wmem_alloc belong to the device being torn
down and are only purged by dev_deactivate_many(), which runs after the
notifier chain returns, so the shutdown itself does not release the waiter.

The pre-patch notifier never blocked on a socket lock.  Would a
bh_lock_sock() plus sock_owned_by_user() check, or deferring pass two to a
work item, avoid holding rtnl_lock across an unbounded socket-lock wait?

> +			if (sk->sk_state == IUCV_CONNECTED) {
>  				if (event == NETDEV_GOING_DOWN)
>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);

[Severity: Medium]
Pass one no longer filters on sk_state, and pass two never re-checks hs_dev
against event_dev under the socket lock.  Can iucv->hs_dev be NULL by the
time iucv_send_ctrl() dereferences it?

net/iucv/af_iucv.c:iucv_send_ctrl() {
	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);
	...
}

The dereference happens before afiucv_hs_send()'s later "if (!skb->dev)"
check can catch it.  A socket collected in IUCV_BOUND state (for example one
blocked in connect() waiting for the SYN-ACK) can reach pass two as
IUCV_CONNECTED with hs_dev already cleared:

net/iucv/af_iucv.c:iucv_sock_close() {
	default:
		iucv_sever_path(sk, 1);      /* IUCV_BOUND: state unchanged */
	}

	if (iucv->hs_dev) {
		dev_put(iucv->hs_dev);
		WRITE_ONCE(iucv->hs_dev, NULL);
	...
}

net/iucv/af_iucv.c:afiucv_hs_callback_synack() {
	if (!iucv || sk->sk_state != IUCV_BOUND) { ... }
	bh_lock_sock(sk);
	...
	sk->sk_state = IUCV_CONNECTED;
}

afiucv_hs_callback_synack() only tests sk_state, so it promotes the closed
but still IUCV_BOUND socket to IUCV_CONNECTED with hs_dev == NULL, and pass
two then calls iucv_send_ctrl() on it.  The same window also lets pass two
call sock_alloc_send_pskb() on an orphaned socket, which dereferences
sk->sk_socket->flags.

The changelog says "BUG-2: iucv_send_ctrl() is now called under lock_sock(),
not racing with concurrent socket close" - should pass two also re-validate
iucv->hs_dev == event_dev under the socket lock?

>  				sk->sk_state = IUCV_DISCONN;

[Severity: Medium]
The changelog states "BUG-3: sk_state is read and written under lock_sock(),
serialising against iucv_sock_close()".  Is that the full picture for
sk_state?

The softirq handlers in this file write sk_state with only bh_lock_sock():

net/iucv/af_iucv.c:afiucv_hs_callback_fin() {
	bh_lock_sock(sk);
	if (sk->sk_state == IUCV_CONNECTED) {
		sk->sk_state = IUCV_DISCONN;
	...
}

and the TX-completion path takes no socket lock at all:

net/iucv/af_iucv.c:afiucv_hs_callback_txnotify() {
	default:
		atomic_dec(&iucv->skbs_in_xmit);
		if (sk->sk_state == IUCV_CONNECTED) {
			sk->sk_state = IUCV_DISCONN;
	...
}

lock_sock() sets sk_lock.owned and then drops sk_lock.slock:

net/core/sock.c:lock_sock_nested() {
	spin_lock_bh(&sk->sk_lock.slock);
	...
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);
}

so bh_lock_sock() users that never test sock_owned_by_user() are not
excluded, and grepping net/iucv/af_iucv.c for sock_owned_by_user() finds no
match.  Can the transition performed here be lost, or clobber a concurrent
softirq transition, while pass two sleeps inside iucv_send_ctrl() ->
sock_alloc_send_skb(GFP_KERNEL)?  Should the BUG-3 claim be scoped to
iucv_sock_close() only?

>  				sk->sk_state_change(sk);
>  			}
> +			release_sock(sk);
> +			sock_put(sk);
> +			list_del(&entry->list);
> +			kfree(entry);
>  		}
>  		break;
>  	case NETDEV_DOWN:

Thanks for looking at these.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917071706.23831-1-nagamani%40linux.ibm.com