[PATCH] igc: fix a log entry using uninitialized netdev

[PATCH] igc: fix a log entry using uninitialized netdev

[Corinna Vinschen]

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(-)

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

[PATCH] igc: fix a log entry using uninitialized netdev

[Hariprasad Kelam]

> 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
> 

Re: [PATCH] igc: fix a log entry using uninitialized netdev

[Andrew Lunn]

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

Re: [PATCH] igc: fix a log entry using uninitialized netdev

[Vinicius Costa Gomes]

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,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH] igc: fix a log entry using uninitialized netdev

[Corinna Vinschen]

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

Re: [Intel-wired-lan] [PATCH] igc: fix a log entry using uninitialized netdev

[Vinicius Costa Gomes]

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,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH] igc: fix a log entry using uninitialized netdev

[naamax.meir]

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>