| Message ID | 20240423102455.901469-1-vinschen@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | igc: fix a log entry using uninitialized netdev | expand |
> During successful probe, igc logs this: > > [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC > added > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > The reason is that igc_ptp_init() is called very early, even before > register_netdev() has been called. So the netdev_info() call works on a > partially uninitialized netdev. > > Fix this by calling igc_ptp_init() after register_netdev(), right after the media > autosense check, just as in igb. Add a comment, just as in igb. > > Now the log message is fine: > > [ 5.200987] igc 0000:01:00.0 eth0: PHC added > Reviewed-by: Hariprasad Kelam <hkelam@marvell.com> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > b/drivers/net/ethernet/intel/igc/igc_main.c > index d9bd001af7ba..e5900d004071 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6927,8 +6927,6 @@ static int igc_probe(struct pci_dev *pdev, > device_set_wakeup_enable(&adapter->pdev->dev, > adapter->flags & > IGC_FLAG_WOL_SUPPORTED); > > - igc_ptp_init(adapter); > - > igc_tsn_clear_schedule(adapter); > > /* reset the hardware with the new settings */ @@ -6950,6 +6948,9 > @@ static int igc_probe(struct pci_dev *pdev, > /* Check if Media Autosense is enabled */ > adapter->ei = *ei; > > + /* do hw tstamp init after resetting */ > + igc_ptp_init(adapter); > + > /* print pcie link status and MAC address */ > pcie_print_link_status(pdev); > netdev_info(netdev, "MAC: %pM\n", netdev->dev_addr); > -- > 2.44.0 >
On Tue, Apr 23, 2024 at 12:24:54PM +0200, Corinna Vinschen wrote: > During successful probe, igc logs this: > > [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > The reason is that igc_ptp_init() is called very early, even before > register_netdev() has been called. So the netdev_info() call works > on a partially uninitialized netdev. > > Fix this by calling igc_ptp_init() after register_netdev(), right > after the media autosense check, just as in igb. Add a comment, > just as in igb. The network stack can start sending and receiving packet before register_netdev() returns. This is typical of NFS root for example. Is there anything in igc_ptp_init() which could cause such packet transfers to explode? A better fix is to allocate the device name earlier. A few drivers call dev_alloc_name(). Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Tue, Apr 23, 2024 at 12:24:54PM +0200, Corinna Vinschen wrote: >> During successful probe, igc logs this: >> >> [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> The reason is that igc_ptp_init() is called very early, even before >> register_netdev() has been called. So the netdev_info() call works >> on a partially uninitialized netdev. >> >> Fix this by calling igc_ptp_init() after register_netdev(), right >> after the media autosense check, just as in igb. Add a comment, >> just as in igb. > > The network stack can start sending and receiving packet before > register_netdev() returns. This is typical of NFS root for example. Is > there anything in igc_ptp_init() which could cause such packet > transfers to explode? > There might be a very narrow window (probably impossible?), what I can see is: 1. the netdevice is exposed to userspace; 2. userspace does the SIOCSHWTSTAMP ioctl() to enable TX timestamps; 3. userspace sends a packet that is going to be timestamped; if this happens before igc_ptp_init() is called, adapter->ptp_tx_lock is going to be uninitialized, and (3) is going to crash. If there's anything that makes this impossible/extremely unlikely, the patch looks good: Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> Cheers,
On Apr 24 17:06, Vinicius Costa Gomes wrote: > Andrew Lunn <andrew@lunn.ch> writes: > > > On Tue, Apr 23, 2024 at 12:24:54PM +0200, Corinna Vinschen wrote: > >> During successful probe, igc logs this: > >> > >> [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> The reason is that igc_ptp_init() is called very early, even before > >> register_netdev() has been called. So the netdev_info() call works > >> on a partially uninitialized netdev. > >> > >> Fix this by calling igc_ptp_init() after register_netdev(), right > >> after the media autosense check, just as in igb. Add a comment, > >> just as in igb. > > > > The network stack can start sending and receiving packet before > > register_netdev() returns. This is typical of NFS root for example. Is > > there anything in igc_ptp_init() which could cause such packet > > transfers to explode? > > > > There might be a very narrow window (probably impossible?), what I can > see is: > > 1. the netdevice is exposed to userspace; > 2. userspace does the SIOCSHWTSTAMP ioctl() to enable TX timestamps; > 3. userspace sends a packet that is going to be timestamped; > > if this happens before igc_ptp_init() is called, adapter->ptp_tx_lock is > going to be uninitialized, and (3) is going to crash. The same would then be possible on igb as well, wouldn't it? > If there's anything that makes this impossible/extremely unlikely, the > patch looks good: > > Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > > > Cheers, > -- > Vinicius Corinna
Corinna Vinschen <vinschen@redhat.com> writes: > On Apr 24 17:06, Vinicius Costa Gomes wrote: >> Andrew Lunn <andrew@lunn.ch> writes: >> >> > On Tue, Apr 23, 2024 at 12:24:54PM +0200, Corinna Vinschen wrote: >> >> During successful probe, igc logs this: >> >> >> >> [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added >> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> The reason is that igc_ptp_init() is called very early, even before >> >> register_netdev() has been called. So the netdev_info() call works >> >> on a partially uninitialized netdev. >> >> >> >> Fix this by calling igc_ptp_init() after register_netdev(), right >> >> after the media autosense check, just as in igb. Add a comment, >> >> just as in igb. >> > >> > The network stack can start sending and receiving packet before >> > register_netdev() returns. This is typical of NFS root for example. Is >> > there anything in igc_ptp_init() which could cause such packet >> > transfers to explode? >> > >> >> There might be a very narrow window (probably impossible?), what I can >> see is: >> >> 1. the netdevice is exposed to userspace; >> 2. userspace does the SIOCSHWTSTAMP ioctl() to enable TX timestamps; >> 3. userspace sends a packet that is going to be timestamped; >> >> if this happens before igc_ptp_init() is called, adapter->ptp_tx_lock is >> going to be uninitialized, and (3) is going to crash. > > The same would then be possible on igb as well, wouldn't it? > Given how many years igb is being used, perhaps "possible" is too strong :-) On igb what exists is slightly different, as there's no ptp_tx_lock there, the "problem" there is trying to enqueue a job on a workqueue that is going to be uninitialized, during this time window. And to be sure, I am still uncertain that this is possible. > >> If there's anything that makes this impossible/extremely unlikely, the >> patch looks good: >> >> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> >> >> Cheers, >> -- >> Vinicius > > > Corinna > Cheers,
On 4/23/2024 13:24, Corinna Vinschen wrote: > During successful probe, igc logs this: > > [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > The reason is that igc_ptp_init() is called very early, even before > register_netdev() has been called. So the netdev_info() call works > on a partially uninitialized netdev. > > Fix this by calling igc_ptp_init() after register_netdev(), right > after the media autosense check, just as in igb. Add a comment, > just as in igb. > > Now the log message is fine: > > [ 5.200987] igc 0000:01:00.0 eth0: PHC added > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Tested-by: Naama Meir <naamax.meir@linux.intel.com>
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index d9bd001af7ba..e5900d004071 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6927,8 +6927,6 @@ static int igc_probe(struct pci_dev *pdev, device_set_wakeup_enable(&adapter->pdev->dev, adapter->flags & IGC_FLAG_WOL_SUPPORTED); - igc_ptp_init(adapter); - igc_tsn_clear_schedule(adapter); /* reset the hardware with the new settings */ @@ -6950,6 +6948,9 @@ static int igc_probe(struct pci_dev *pdev, /* Check if Media Autosense is enabled */ adapter->ei = *ei; + /* do hw tstamp init after resetting */ + igc_ptp_init(adapter); + /* print pcie link status and MAC address */ pcie_print_link_status(pdev); netdev_info(netdev, "MAC: %pM\n", netdev->dev_addr);
During successful probe, igc logs this: [ 5.133667] igc 0000:01:00.0 (unnamed net_device) (uninitialized): PHC added ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The reason is that igc_ptp_init() is called very early, even before register_netdev() has been called. So the netdev_info() call works on a partially uninitialized netdev. Fix this by calling igc_ptp_init() after register_netdev(), right after the media autosense check, just as in igb. Add a comment, just as in igb. Now the log message is fine: [ 5.200987] igc 0000:01:00.0 eth0: PHC added Signed-off-by: Corinna Vinschen <vinschen@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)