netdev - Re: [net-next v2 14/17] i40e: enable PTP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 10 Jan 2014 22:08:46 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	"Keller, Jacob E" <jacob.e.keller@...el.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	Richard Cochran <richardcochran@...il.com>,
	"Vick, Matthew" <matthew.vick@...el.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
Subject: Re: [net-next v2 14/17] i40e: enable PTP

On Fri, 2014-01-10 at 21:42 +0000, Keller, Jacob E wrote:
> On Fri, 2014-01-10 at 20:37 +0000, Ben Hutchings wrote:
> > On Fri, 2014-01-10 at 12:30 -0800, Jeff Kirsher wrote:
> > > From: Jacob Keller <jacob.e.keller@...el.com>
> > > 
> > > New feature: Enable PTP support in the i40e driver.
> > > 
> > > Change-ID: I6a8e799f582705191f9583afb1b9231a8db96cc8
> > > Cc: Richard Cochran <richardcochran@...il.com>
> > > Cc: Ben Hutchings <bhutchings@...arflare.com>
> > > Signed-off-by: Matthew Vick <matthew.vick@...el.com>
> > > Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > > ---
> > >  drivers/net/ethernet/intel/Kconfig             |   1 +
> > >  drivers/net/ethernet/intel/i40e/Makefile       |   1 +
> > >  drivers/net/ethernet/intel/i40e/i40e.h         |  26 +
> > >  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  33 +-
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c    |  47 +-
> > >  drivers/net/ethernet/intel/i40e/i40e_ptp.c     | 662 +++++++++++++++++++++++++
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c    |  53 ++
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.h    |   3 +
> > >  8 files changed, 824 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > > 
> > > diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> > > index 9fb2eb8..333bb54 100644
> > > --- a/drivers/net/ethernet/intel/Kconfig
> > > +++ b/drivers/net/ethernet/intel/Kconfig
> > [...]
> > > +void i40e_ptp_init(struct i40e_pf *pf)
> > > +{
> > > +	struct i40e_hw *hw = &pf->hw;
> > > +	struct net_device *netdev = pf->vsi[pf->lan_vsi]->netdev;
> > > +
> > > +	strncpy(pf->ptp_caps.name, netdev->name, sizeof(pf->ptp_caps.name));
> > [...]
> > 
> > I recommended using the *driver* name as the clock name.  The net device
> > doesn't even have a proper name at this point.

Actually, on closer inspection it looks like the net device gets
registered before this is called, so the name will be valid.  However
the net device can (and probably will) be renamed after registration so
this name may become stale.

If the net device is registered first, this raises a different problem:
i40e_get_ts_info() can then race with PTP init and cleanup; it can see a
non-null but invalid value of pf->ptp_clock if it races with this in
i40e_ptp_init():

> +	/* Attempt to register the clock before enabling the hardware. */
> +	pf->ptp_clock = ptp_clock_register(&pf->ptp_caps, &pf->pdev->dev);
> +	if (IS_ERR(pf->ptp_clock)) {

or it can see a non-null value of ptp_clock just before the clock is
freed, then use-after-free.

You could use rtnl_lock() to serialise the registration/unregistration
with i40e_get_ts_info(), or reorder init and cleanup so the clock is
registered before the net device and unregistered after.

> > Ben.
> > 
> 
> We use netdev->name a few lines farther down.. should that be changed as
> well?

I think that's fine if it's done after the net device is registered.
However you would normally use netdev_info() to include the name at the
beginning of the log line.

Ben.

> Sorry about this one. The ixgbe driver initializes clock at open
> instead of at init, so it has a valid clock name already.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html