| 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: <IA1PR11MB628961151DD523FECB144DD289D2A@IA1PR11MB6289.namprd11.prod.outlook.com>
Date: Sat, 22 Nov 2025 05:03:53 +0000
From: "Joshi, Sreedevi" <sreedevi.joshi@...el.com>
To: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Samudrala, Sridhar"
<sridhar.samudrala@...el.com>, "Tantilov, Emil S" <emil.s.tantilov@...el.com>
Subject: RE: [PATCH iwl-net 1/3] idpf: Fix RSS LUT NULL pointer crash on early
ethtool operations
> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
> Sent: Friday, November 21, 2025 4:22 PM
> To: Joshi, Sreedevi <sreedevi.joshi@...el.com>; intel-wired-lan@...ts.osuosl.org
> Cc: netdev@...r.kernel.org; Samudrala, Sridhar <sridhar.samudrala@...el.com>; Tantilov, Emil S <emil.s.tantilov@...el.com>
> Subject: Re: [PATCH iwl-net 1/3] idpf: Fix RSS LUT NULL pointer crash on early ethtool operations
>
>
>
> On 11/17/2025 8:22 PM, Sreedevi Joshi wrote:
> > The RSS LUT is not initialized until the interface comes up, causing
> > the following NULL pointer crash when ethtool operations like rxhash on/off
> > are performed before the interface is brought up for the first time.
> >
> > Move RSS LUT initialization from ndo_open to vport creation to ensure LUT
> > is always available. This enables RSS configuration via ethtool before
> > bringing the interface up. Simplify LUT management by maintaining all
> > changes in the driver's soft copy and programming zeros to the indirection
> > table when rxhash is disabled. Defer HW programming until the interface
> > comes up if it is down during rxhash and LUT configuration changes.
> >
> > [89408.371875] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [89408.371908] #PF: supervisor read access in kernel mode
> > [89408.371924] #PF: error_code(0x0000) - not-present page
> > [89408.371940] PGD 0 P4D 0
> > [89408.371953] Oops: Oops: 0000 [#1] SMP NOPTI
> > <snip>
> > [89408.372052] RIP: 0010:memcpy_orig+0x16/0x130
> > [89408.372310] Call Trace:
> > [89408.372317] <TASK>
> > [89408.372326] ? idpf_set_features+0xfc/0x180 [idpf]
> > [89408.372363] __netdev_update_features+0x295/0xde0
> > [89408.372384] ethnl_set_features+0x15e/0x460
> > [89408.372406] genl_family_rcv_msg_doit+0x11f/0x180
> > [89408.372429] genl_rcv_msg+0x1ad/0x2b0
> > [89408.372446] ? __pfx_ethnl_set_features+0x10/0x10
> > [89408.372465] ? __pfx_genl_rcv_msg+0x10/0x10
> > [89408.372482] netlink_rcv_skb+0x58/0x100
> > [89408.372502] genl_rcv+0x2c/0x50
> > [89408.372516] netlink_unicast+0x289/0x3e0
> > [89408.372533] netlink_sendmsg+0x215/0x440
> > [89408.372551] __sys_sendto+0x234/0x240
> > [89408.372571] __x64_sys_sendto+0x28/0x30
> > [89408.372585] x64_sys_call+0x1909/0x1da0
> > [89408.372604] do_syscall_64+0x7a/0xfa0
> > [89408.373140] ? clear_bhb_loop+0x60/0xb0
> > [89408.373647] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > [89408.378887] </TASK>
> > <snip>
> >
> > Fixes: a251eee62133 ("idpf: add SRIOV support and other ndo_ops")
> > Signed-off-by: Sreedevi Joshi <sreedevi.joshi@...el.com>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> > Reviewed-by: Emil Tantilov <emil.s.tantilov@...el.com>
> > ---
> > drivers/net/ethernet/intel/idpf/idpf.h | 2 -
> > drivers/net/ethernet/intel/idpf/idpf_lib.c | 89 +++++++++----------
> > drivers/net/ethernet/intel/idpf/idpf_txrx.c | 36 +++-----
> > drivers/net/ethernet/intel/idpf/idpf_txrx.h | 4 +-
> > .../net/ethernet/intel/idpf/idpf_virtchnl.c | 9 +-
> > 5 files changed, 64 insertions(+), 76 deletions(-)
> >
>
> ...
>
>
> > @@ -1289,6 +1291,13 @@ static struct idpf_vport *idpf_vport_alloc(struct idpf_adapter *adapter,
> > /* Initialize default rss key */
> > netdev_rss_key_fill((void *)rss_data->rss_key, rss_data->rss_key_size);
> >
> > + /* Initialize default rss LUT */
> > + err = idpf_init_rss_lut(vport);
> > + if (err) {
> > + kfree(rss_data->rss_key);
>
> Can you move this free into the goto path? In case anything new gets
> added in the future, it'll already be there.
>
Will do.
> > + goto free_vport;
>
> The previous error/goto goes to free_vector_idxs. Would this goto leak
> q_vector_idxs?
>
Good catch! Will fix.
> > + }
> > +
> > /* fill vport slot in the adapter struct */
> > adapter->vports[idx] = vport;
> > adapter->vport_ids[idx] = idpf_get_vport_id(vport);
>
> ...
>
> > @@ -1593,7 +1612,7 @@ static int idpf_vport_open(struct idpf_vport *vport, bool rtnl)
> > return 0;
> >
> > deinit_rss:
> > - idpf_deinit_rss(vport);
> > + idpf_deinit_rss_lut(vport);
>
> Since this patch moved init out of open, should this be moved out too?
>
Wanted to have a corresponding deinit when init is done in this function. However, I think it is ok to not deinit here.
The logic will be updated for soft reset logic anyway to not dealloc the rss_lut in patch3 and then there is no need for
init from this function.
While reviewing all the goto's in this function, I found two more changes that will be required. Shall
create a separate patch for this? (they are not directly related to the RSS)
@@ -1526,14 +1526,14 @@ static int idpf_vport_open(struct idpf_vport *vport, bool rtnl)
if (err) {
dev_err(&adapter->pdev->dev, "Failed to initialize queue registers for vport %u: %d\n",
vport->vport_id, err);
- goto queues_rel;
+ goto intr_deinit;
}
err = idpf_rx_bufs_init_all(vport);
if (err) {
dev_err(&adapter->pdev->dev, "Failed to initialize RX buffers for vport %u: %d\n",
vport->vport_id, err);
- goto queues_rel;
+ goto intr_deinit;
}
> > disable_vport:
> > idpf_send_disable_vport_msg(vport);
> > disable_queues:
> >
>
> ...
>
> > @@ -2839,7 +2845,8 @@ int idpf_send_get_set_rss_lut_msg(struct idpf_vport *vport, bool get)
> > } else {
> > rl->lut_entries = cpu_to_le16(rss_data->rss_lut_size);
> > for (i = 0; i < rss_data->rss_lut_size; i++)
> > - rl->lut[i] = cpu_to_le32(rss_data->rss_lut[i]);
> > + rl->lut[i] = (rxhash_ena) ?
>
> The parens don't look needed.
>
Will fix.
Thanks for the review, Tony!
Regards,
Sreedevi
> Thanks,
> Tony
>
> > + cpu_to_le32(rss_data->rss_lut[i]) : 0;
> >
> > xn_params.vc_op = VIRTCHNL2_OP_SET_RSS_LUT;
> > }
Powered by blists - more mailing lists