lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <d2351c0f-0bfe-9422-f6f3-f0a0db58c729@sangfor.com.cn> Date: Wed, 3 May 2023 22:00:49 +0800 From: Ding Hui <dinghui@...gfor.com.cn> To: Leon Romanovsky <leon@...nel.org> Cc: dinghui@...gfor.com.cn, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, intel-wired-lan@...ts.osuosl.org, jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com, keescook@...omium.org, grzegorzx.szczurek@...el.com, mateusz.palczewski@...el.com, mitch.a.williams@...el.com, gregory.v.rose@...el.com, jeffrey.t.kirsher@...el.com, michal.kubiak@...el.com, simon.horman@...igine.com, madhu.chittim@...el.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, pengdonglin@...gfor.com.cn, huangcun@...gfor.com.cn Subject: Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels on remove On 2023/5/3 4:24 下午, Leon Romanovsky wrote: > On Wed, May 03, 2023 at 11:15:41AM +0800, Ding Hui wrote: >> >> If we detected removing is in processing, we can avoid unnecessary >> waiting and return error faster. >> >> On the other hand in timeout handling, we should keep the original >> num_active_queues and reset num_req_queues to 0. >> >> Fixes: 4e5e6b5d9d13 ("iavf: Fix return of set the new channel count") >> Signed-off-by: Ding Hui <dinghui@...gfor.com.cn> >> Cc: Donglin Peng <pengdonglin@...gfor.com.cn> >> Cc: Huang Cun <huangcun@...gfor.com.cn> >> Reviewed-by: Simon Horman <simon.horman@...igine.com> >> Reviewed-by: Michal Kubiak <michal.kubiak@...el.com> >> --- >> v3 to v4: >> - nothing changed >> >> v2 to v3: >> - fix review tag >> >> v1 to v2: >> - add reproduction script >> >> --- >> drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c >> index 6f171d1d85b7..d8a3c0cfedd0 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c >> @@ -1857,13 +1857,15 @@ static int iavf_set_channels(struct net_device *netdev, >> /* wait for the reset is done */ >> for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) { >> msleep(IAVF_RESET_WAIT_MS); >> + if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) >> + return -EOPNOTSUPP; > > This makes no sense without locking as change to __IAVF_IN_REMOVE_TASK > can happen any time. > The state doesn't need to be that precise here, it is optimized only for the fast path. During the lifecycle of the adapter, the __IAVF_IN_REMOVE_TASK state will only be set and not cleared. If we didn't detect the "removing" state, we also can fallback to timeout handling. So I don't think the locking is necessary here, what do the maintainers at Intel think? > Thanks > >> if (adapter->flags & IAVF_FLAG_RESET_PENDING) >> continue; >> break; >> } >> if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) { >> adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED; >> - adapter->num_active_queues = num_req; >> + adapter->num_req_queues = 0; >> return -EOPNOTSUPP; >> } >> >> -- >> 2.17.1 >> >> > -- Thanks, -dinghui
Powered by blists - more mailing lists