All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
To: Andrew Lunn <andrew@lunn.ch>, Corinna Vinschen <vinschen@redhat.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH] igc: fix a log entry using uninitialized netdev
Date: Wed, 24 Apr 2024 17:06:04 -0700	[thread overview]
Message-ID: <8734raxq4z.fsf@intel.com> (raw)
In-Reply-To: <033cce07-fe8f-42e6-8c27-7afee87fe13c@lunn.ch>

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

WARNING: multiple messages have this Message-ID (diff)
From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
To: Andrew Lunn <andrew@lunn.ch>, Corinna Vinschen <vinschen@redhat.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org
Subject: Re: [PATCH] igc: fix a log entry using uninitialized netdev
Date: Wed, 24 Apr 2024 17:06:04 -0700	[thread overview]
Message-ID: <8734raxq4z.fsf@intel.com> (raw)
In-Reply-To: <033cce07-fe8f-42e6-8c27-7afee87fe13c@lunn.ch>

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

  reply	other threads:[~2024-04-25  0:06 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-23 10:24 [Intel-wired-lan] [PATCH] igc: fix a log entry using uninitialized netdev Corinna Vinschen
2024-04-23 10:24 ` Corinna Vinschen
2024-04-23 10:49 ` [Intel-wired-lan] " Hariprasad Kelam
2024-04-23 10:49   ` Hariprasad Kelam
2024-04-24 22:34 ` [Intel-wired-lan] " Andrew Lunn
2024-04-24 22:34   ` Andrew Lunn
2024-04-25  0:06   ` Vinicius Costa Gomes [this message]
2024-04-25  0:06     ` Vinicius Costa Gomes
2024-04-25  7:47     ` [Intel-wired-lan] " Corinna Vinschen
2024-04-25  7:47       ` Corinna Vinschen
2024-04-25 18:46       ` Vinicius Costa Gomes
2024-04-25 18:46         ` Vinicius Costa Gomes
2024-05-07  9:00 ` naamax.meir

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8734raxq4z.fsf@intel.com \
    --to=vinicius.gomes@intel.com \
    --cc=andrew@lunn.ch \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=netdev@vger.kernel.org \
    --cc=vinschen@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.