Skip to content

Commit 4a53566

Browse files
caringicaringi
committed
Merge: Updates for ibmveth pool store
MR: https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/6946 Description: Updates for ibmveth pool store JIRA: https://issues.redhat.com/browse/RHEL-93830 Build Info: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=67820140 Tested: Verified Brew build test kernel RPMs and confirmed issue is resovled Signed-off-by: Mamatha Inamdar <minamdar@redhat.com> commit 053f3ff Author: Dave Marquardt <davemarq@linux.ibm.com> Date: Wed Apr 2 10:44:03 2025 -0500 net: ibmveth: make veth_pool_store stop hanging v2: - Created a single error handling unlock and exit in veth_pool_store - Greatly expanded commit message with previous explanatory-only text Summary: Use rtnl_mutex to synchronize veth_pool_store with itself, ibmveth_close and ibmveth_open, preventing multiple calls in a row to napi_disable. Background: Two (or more) threads could call veth_pool_store through writing to /sys/devices/vio/30000002/pool*/*. You can do this easily with a little shell script. This causes a hang. I configured LOCKDEP, compiled ibmveth.c with DEBUG, and built a new kernel. I ran this test again and saw: Setting pool0/active to 0 Setting pool1/active to 1 [ 73.911067][ T4365] ibmveth 30000002 eth0: close starting Setting pool1/active to 1 Setting pool1/active to 0 [ 73.911367][ T4366] ibmveth 30000002 eth0: close starting [ 73.916056][ T4365] ibmveth 30000002 eth0: close complete [ 73.916064][ T4365] ibmveth 30000002 eth0: open starting [ 110.808564][ T712] systemd-journald[712]: Sent WATCHDOG=1 notification. [ 230.808495][ T712] systemd-journald[712]: Sent WATCHDOG=1 notification. [ 243.683786][ T123] INFO: task stress.sh:4365 blocked for more than 122 seconds. [ 243.683827][ T123] Not tainted 6.14.0-01103-g2df0c02dab82-dirty #8 [ 243.683833][ T123] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 243.683838][ T123] task:stress.sh state:D stack:28096 pid:4365 tgid:4365 ppid:4364 task_flags:0x400040 flags:0x00042000 [ 243.683852][ T123] Call Trace: [ 243.683857][ T123] [c00000000c38f690] [0000000000000001] 0x1 (unreliable) [ 243.683868][ T123] [c00000000c38f840] [c00000000001f908] __switch_to+0x318/0x4e0 [ 243.683878][ T123] [c00000000c38f8a0] [c000000001549a70] __schedule+0x500/0x12a0 [ 243.683888][ T123] [c00000000c38f9a0] [c00000000154a878] schedule+0x68/0x210 [ 243.683896][ T123] [c00000000c38f9d0] [c00000000154ac80] schedule_preempt_disabled+0x30/0x50 [ 243.683904][ T123] [c00000000c38fa00] [c00000000154dbb0] __mutex_lock+0x730/0x10f0 [ 243.683913][ T123] [c00000000c38fb10] [c000000001154d40] napi_enable+0x30/0x60 [ 243.683921][ T123] [c00000000c38fb40] [c000000000f4ae94] ibmveth_open+0x68/0x5dc [ 243.683928][ T123] [c00000000c38fbe0] [c000000000f4aa20] veth_pool_store+0x220/0x270 [ 243.683936][ T123] [c00000000c38fc70] [c000000000826278] sysfs_kf_write+0x68/0xb0 [ 243.683944][ T123] [c00000000c38fcb0] [c0000000008240b8] kernfs_fop_write_iter+0x198/0x2d0 [ 243.683951][ T123] [c00000000c38fd00] [c00000000071b9ac] vfs_write+0x34c/0x650 [ 243.683958][ T123] [c00000000c38fdc0] [c00000000071bea8] ksys_write+0x88/0x150 [ 243.683966][ T123] [c00000000c38fe10] [c0000000000317f4] system_call_exception+0x124/0x340 [ 243.683973][ T123] [c00000000c38fe50] [c00000000000d05c] system_call_vectored_common+0x15c/0x2ec ... [ 243.684087][ T123] Showing all locks held in the system: [ 243.684095][ T123] 1 lock held by khungtaskd/123: [ 243.684099][ T123] #0: c00000000278e370 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x50/0x248 [ 243.684114][ T123] 4 locks held by stress.sh/4365: [ 243.684119][ T123] #0: c00000003a4cd3f8 (sb_writers#3){.+.+}-{0:0}, at: ksys_write+0x88/0x150 [ 243.684132][ T123] #1: c000000041aea888 (&of->mutex#2){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x154/0x2d0 [ 243.684143][ T123] #2: c0000000366fb9a8 (kn->active#64){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x160/0x2d0 [ 243.684155][ T123] #3: c000000035ff4cb8 (&dev->lock){+.+.}-{3:3}, at: napi_enable+0x30/0x60 [ 243.684166][ T123] 5 locks held by stress.sh/4366: [ 243.684170][ T123] #0: c00000003a4cd3f8 (sb_writers#3){.+.+}-{0:0}, at: ksys_write+0x88/0x150 [ 243.684183][ T123] #1: c00000000aee2288 (&of->mutex#2){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x154/0x2d0 [ 243.684194][ T123] #2: c0000000366f4ba8 (kn->active#64){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x160/0x2d0 [ 243.684205][ T123] #3: c000000035ff4cb8 (&dev->lock){+.+.}-{3:3}, at: napi_disable+0x30/0x60 [ 243.684216][ T123] #4: c0000003ff9bbf18 (&rq->__lock){-.-.}-{2:2}, at: __schedule+0x138/0x12a0 From the ibmveth debug, two threads are calling veth_pool_store, which calls ibmveth_close and ibmveth_open. Here's the sequence: T4365 T4366 ----------------- ----------------- --------- veth_pool_store veth_pool_store ibmveth_close ibmveth_close napi_disable napi_disable ibmveth_open napi_enable <- HANG ibmveth_close calls napi_disable at the top and ibmveth_open calls napi_enable at the top. https://docs.kernel.org/networking/napi.html]] says The control APIs are not idempotent. Control API calls are safe against concurrent use of datapath APIs but an incorrect sequence of control API calls may result in crashes, deadlocks, or race conditions. For example, calling napi_disable() multiple times in a row will deadlock. In the normal open and close paths, rtnl_mutex is acquired to prevent other callers. This is missing from veth_pool_store. Use rtnl_mutex in veth_pool_store fixes these hangs. Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com> Fixes: 860f242 ("[PATCH] ibmveth change buffer pools dynamically") Reviewed-by: Nick Child <nnac123@linux.ibm.com> Reviewed-by: Simon Horman <horms@kernel.org> Link: https://patch.msgid.link/20250402154403.386744-1-davemarq@linux.ibm.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Mamatha Inamdar <minamdar@redhat.com> Approved-by: Steve Best <sbest@redhat.com> Approved-by: Ivan Vecera <ivecera@redhat.com> Approved-by: CKI KWF Bot <cki-ci-bot+kwf-gitlab-com@redhat.com> Merged-by: Augusto Caringi <acaringi@redhat.com>
2 parents 0abf540 + bb44be4 commit 4a53566

File tree

2 files changed

+28
-30
lines changed

2 files changed

+28
-30
lines changed

‎drivers/net/ethernet/ibm/ibmveth.c

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -667,8 +667,7 @@ static int ibmveth_close(struct net_device *netdev)
667667

668668
napi_disable(&adapter->napi);
669669

670-
if (!adapter->pool_config)
671-
netif_tx_stop_all_queues(netdev);
670+
netif_tx_stop_all_queues(netdev);
672671

673672
h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
674673

@@ -776,9 +775,7 @@ static int ibmveth_set_csum_offload(struct net_device *dev, u32 data)
776775

777776
if (netif_running(dev)) {
778777
restart = 1;
779-
adapter->pool_config = 1;
780778
ibmveth_close(dev);
781-
adapter->pool_config = 0;
782779
}
783780

784781
set_attr = 0;
@@ -860,9 +857,7 @@ static int ibmveth_set_tso(struct net_device *dev, u32 data)
860857

861858
if (netif_running(dev)) {
862859
restart = 1;
863-
adapter->pool_config = 1;
864860
ibmveth_close(dev);
865-
adapter->pool_config = 0;
866861
}
867862

868863
set_attr = 0;
@@ -1512,9 +1507,7 @@ static int ibmveth_change_mtu(struct net_device *dev, int new_mtu)
15121507
only the buffer pools necessary to hold the new MTU */
15131508
if (netif_running(adapter->netdev)) {
15141509
need_restart = 1;
1515-
adapter->pool_config = 1;
15161510
ibmveth_close(adapter->netdev);
1517-
adapter->pool_config = 0;
15181511
}
15191512

15201513
/* Look for an active buffer pool that can hold the new MTU */
@@ -1678,7 +1671,6 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
16781671
adapter->vdev = dev;
16791672
adapter->netdev = netdev;
16801673
adapter->mcastFilterSize = be32_to_cpu(*mcastFilterSize_p);
1681-
adapter->pool_config = 0;
16821674
ibmveth_init_link_settings(netdev);
16831675

16841676
netif_napi_add_weight(netdev, &adapter->napi, ibmveth_poll, 16);
@@ -1810,20 +1802,22 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
18101802
long value = simple_strtol(buf, NULL, 10);
18111803
long rc;
18121804

1805+
rtnl_lock();
1806+
18131807
if (attr == &veth_active_attr) {
18141808
if (value && !pool->active) {
18151809
if (netif_running(netdev)) {
18161810
if (ibmveth_alloc_buffer_pool(pool)) {
18171811
netdev_err(netdev,
18181812
"unable to alloc pool\n");
1819-
return -ENOMEM;
1813+
rc = -ENOMEM;
1814+
goto unlock_err;
18201815
}
18211816
pool->active = 1;
1822-
adapter->pool_config = 1;
18231817
ibmveth_close(netdev);
1824-
adapter->pool_config = 0;
1825-
if ((rc = ibmveth_open(netdev)))
1826-
return rc;
1818+
rc = ibmveth_open(netdev);
1819+
if (rc)
1820+
goto unlock_err;
18271821
} else {
18281822
pool->active = 1;
18291823
}
@@ -1843,54 +1837,59 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
18431837

18441838
if (i == IBMVETH_NUM_BUFF_POOLS) {
18451839
netdev_err(netdev, "no active pool >= MTU\n");
1846-
return -EPERM;
1840+
rc = -EPERM;
1841+
goto unlock_err;
18471842
}
18481843

18491844
if (netif_running(netdev)) {
1850-
adapter->pool_config = 1;
18511845
ibmveth_close(netdev);
18521846
pool->active = 0;
1853-
adapter->pool_config = 0;
1854-
if ((rc = ibmveth_open(netdev)))
1855-
return rc;
1847+
rc = ibmveth_open(netdev);
1848+
if (rc)
1849+
goto unlock_err;
18561850
}
18571851
pool->active = 0;
18581852
}
18591853
} else if (attr == &veth_num_attr) {
18601854
if (value <= 0 || value > IBMVETH_MAX_POOL_COUNT) {
1861-
return -EINVAL;
1855+
rc = -EINVAL;
1856+
goto unlock_err;
18621857
} else {
18631858
if (netif_running(netdev)) {
1864-
adapter->pool_config = 1;
18651859
ibmveth_close(netdev);
1866-
adapter->pool_config = 0;
18671860
pool->size = value;
1868-
if ((rc = ibmveth_open(netdev)))
1869-
return rc;
1861+
rc = ibmveth_open(netdev);
1862+
if (rc)
1863+
goto unlock_err;
18701864
} else {
18711865
pool->size = value;
18721866
}
18731867
}
18741868
} else if (attr == &veth_size_attr) {
18751869
if (value <= IBMVETH_BUFF_OH || value > IBMVETH_MAX_BUF_SIZE) {
1876-
return -EINVAL;
1870+
rc = -EINVAL;
1871+
goto unlock_err;
18771872
} else {
18781873
if (netif_running(netdev)) {
1879-
adapter->pool_config = 1;
18801874
ibmveth_close(netdev);
1881-
adapter->pool_config = 0;
18821875
pool->buff_size = value;
1883-
if ((rc = ibmveth_open(netdev)))
1884-
return rc;
1876+
rc = ibmveth_open(netdev);
1877+
if (rc)
1878+
goto unlock_err;
18851879
} else {
18861880
pool->buff_size = value;
18871881
}
18881882
}
18891883
}
1884+
rtnl_unlock();
18901885

18911886
/* kick the interrupt handler to allocate/deallocate pools */
18921887
ibmveth_interrupt(netdev->irq, netdev);
18931888
return count;
1889+
1890+
unlock_err:
1891+
rtnl_unlock();
1892+
return rc;
18941893
}
18951894

18961895

‎drivers/net/ethernet/ibm/ibmveth.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,6 @@ struct ibmveth_adapter {
147147
dma_addr_t filter_list_dma;
148148
struct ibmveth_buff_pool rx_buff_pool[IBMVETH_NUM_BUFF_POOLS];
149149
struct ibmveth_rx_q rx_queue;
150-
int pool_config;
151150
int rx_csum;
152151
int large_send;
153152
bool is_active_trunk;

0 commit comments

Comments
 (0)