> -----Original Message----- > From: Nguyen, Anthony L <[email protected]> > Sent: Friday, November 21, 2025 4:22 PM > To: Joshi, Sreedevi <[email protected]>; > [email protected] > Cc: [email protected]; Samudrala, Sridhar <[email protected]>; > Tantilov, Emil S <[email protected]> > 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 <[email protected]> > > Reviewed-by: Sridhar Samudrala <[email protected]> > > Reviewed-by: Emil Tantilov <[email protected]> > > --- > > 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; > > }
