| Message ID | 20240826181032.3042222-3-manojvishy@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [[PATCH,v2,iwl-next] v2 2/4] idpf: Acquire the lock before accessing the xn->salt | expand |
On 8/26/2024 11:10 AM, Manoj Vishwanathan wrote: > The transaction salt was being accessed before acquiring the > idpf_vc_xn_lock when idpf has to forward the virtchnl reply. > > Fixes: 34c21fa894a1a (“idpf: implement virtchnl transaction manager”) > Signed-off-by: Manoj Vishwanathan <manojvishy@google.com> > --- Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > index 70986e12da28..30eec674d594 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > @@ -612,14 +612,15 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter, > return -EINVAL; > } > xn = &adapter->vcxn_mngr->ring[xn_idx]; > + idpf_vc_xn_lock(xn); Could look at implementing cleanup.h based locking here so that we could use guard or scope_guard and not have to litter the exit paths with unlocks. I don't think that needs to be done in this patch, though. > salt = FIELD_GET(IDPF_VC_XN_SALT_M, msg_info); > if (xn->salt != salt) { > dev_err_ratelimited(&adapter->pdev->dev, "Transaction salt does not match (%02x != %02x)\n", > xn->salt, salt); > + idpf_vc_xn_unlock(xn); > return -EINVAL; > } > > - idpf_vc_xn_lock(xn); > switch (xn->state) { > case IDPF_VC_XN_WAITING: > /* success */
On 8/26/2024 11:10 AM, Manoj Vishwanathan wrote: > The transaction salt was being accessed before acquiring the > idpf_vc_xn_lock when idpf has to forward the virtchnl reply. > > Fixes: 34c21fa894a1a (“idpf: implement virtchnl transaction manager”) > Signed-off-by: Manoj Vishwanathan <manojvishy@google.com> Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com> > --- > drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > index 70986e12da28..30eec674d594 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > @@ -612,14 +612,15 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter, > return -EINVAL; > } > xn = &adapter->vcxn_mngr->ring[xn_idx]; > + idpf_vc_xn_lock(xn); > salt = FIELD_GET(IDPF_VC_XN_SALT_M, msg_info); > if (xn->salt != salt) { > dev_err_ratelimited(&adapter->pdev->dev, "Transaction salt does not match (%02x != %02x)\n", > xn->salt, salt); > + idpf_vc_xn_unlock(xn); > return -EINVAL; > } > > - idpf_vc_xn_lock(xn); > switch (xn->state) { > case IDPF_VC_XN_WAITING: > /* success */
On 8/28/24 23:29, Jacob Keller wrote: > > > On 8/26/2024 11:10 AM, Manoj Vishwanathan wrote: >> The transaction salt was being accessed before acquiring the >> idpf_vc_xn_lock when idpf has to forward the virtchnl reply. >> >> Fixes: 34c21fa894a1a (“idpf: implement virtchnl transaction manager”) >> Signed-off-by: Manoj Vishwanathan <manojvishy@google.com> >> --- > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > >> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> index 70986e12da28..30eec674d594 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> @@ -612,14 +612,15 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter, >> return -EINVAL; >> } >> xn = &adapter->vcxn_mngr->ring[xn_idx]; >> + idpf_vc_xn_lock(xn); > > Could look at implementing cleanup.h based locking here so that we could > use guard or scope_guard and not have to litter the exit paths with unlocks. only scope_guard() for networking code > > I don't think that needs to be done in this patch, though. +1 > >> salt = FIELD_GET(IDPF_VC_XN_SALT_M, msg_info); >> if (xn->salt != salt) { >> dev_err_ratelimited(&adapter->pdev->dev, "Transaction salt does not match (%02x != %02x)\n", >> xn->salt, salt); >> + idpf_vc_xn_unlock(xn); >> return -EINVAL; >> } >> >> - idpf_vc_xn_lock(xn); >> switch (xn->state) { >> case IDPF_VC_XN_WAITING: >> /* success */
> -----Original Message----- > From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Sent: Thursday, August 29, 2024 11:05 PM > To: Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; google-lan- > reviews@googlegroups.com; Manoj Vishwanathan <manojvishy@google.com>; > David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [[PATCH v2 iwl-next] v2 2/4] idpf: Acquire the lock > before accessing the xn->salt > > On 8/28/24 23:29, Jacob Keller wrote: > > > > > > On 8/26/2024 11:10 AM, Manoj Vishwanathan wrote: > >> The transaction salt was being accessed before acquiring the > >> idpf_vc_xn_lock when idpf has to forward the virtchnl reply. > >> > >> Fixes: 34c21fa894a1a (“idpf: implement virtchnl transaction manager”) > >> Signed-off-by: Manoj Vishwanathan <manojvishy@google.com> > >> --- > > > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > > >> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > >> index 70986e12da28..30eec674d594 100644 > >> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > >> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > >> @@ -612,14 +612,15 @@ idpf_vc_xn_forward_reply(struct idpf_adapter > *adapter, > >> return -EINVAL; > >> } > >> xn = &adapter->vcxn_mngr->ring[xn_idx]; > >> + idpf_vc_xn_lock(xn); > > > > Could look at implementing cleanup.h based locking here so that we could > > use guard or scope_guard and not have to litter the exit paths with unlocks. > > only scope_guard() for networking code > Yea, leaving it as-is is fine. I personally find cleanup-based locking better, but it appears the maintainers and majority feel otherwise.
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c index 70986e12da28..30eec674d594 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c @@ -612,14 +612,15 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter, return -EINVAL; } xn = &adapter->vcxn_mngr->ring[xn_idx]; + idpf_vc_xn_lock(xn); salt = FIELD_GET(IDPF_VC_XN_SALT_M, msg_info); if (xn->salt != salt) { dev_err_ratelimited(&adapter->pdev->dev, "Transaction salt does not match (%02x != %02x)\n", xn->salt, salt); + idpf_vc_xn_unlock(xn); return -EINVAL; } - idpf_vc_xn_lock(xn); switch (xn->state) { case IDPF_VC_XN_WAITING: /* success */
The transaction salt was being accessed before acquiring the idpf_vc_xn_lock when idpf has to forward the virtchnl reply. Fixes: 34c21fa894a1a (“idpf: implement virtchnl transaction manager”) Signed-off-by: Manoj Vishwanathan <manojvishy@google.com> --- drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)