netdev - RE: [net-next v2 2/2] net: reject ptp requests with unsupported flags

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <02874ECE860811409154E81DA85FBB58968D86AE@ORSMSX121.amr.corp.intel.com>
Date:   Thu, 26 Sep 2019 17:41:26 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Richard Cochran <richardcochran@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        "David S . Miller" <davem@...emloft.net>,
        "Hall, Christopher S" <christopher.s.hall@...el.com>
Subject: RE: [net-next v2 2/2] net: reject ptp requests with unsupported
 flags



> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org] On
> Behalf Of Richard Cochran
> Sent: Wednesday, September 25, 2019 9:02 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: netdev@...r.kernel.org; Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>;
> Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; Felipe Balbi
> <felipe.balbi@...ux.intel.com>; David S . Miller <davem@...emloft.net>; Hall,
> Christopher S <christopher.s.hall@...el.com>
> Subject: Re: [net-next v2 2/2] net: reject ptp requests with unsupported flags
> 
> On Wed, Sep 25, 2019 at 07:28:20PM -0700, Jacob Keller wrote:
> > This patch may not be correct for individual drivers, especially
> > regarding the rising vs falling edge flags. I interpreted the default
> > behavior to be to timestamp the rising edge of a pin transition.
> 
> So I think this patch goes too far.  It breaks the implied ABI.

Sure, I didn't really know whether the rising vs falling edge and how it was supposed to work for each driver. I just want to ensure that any future flags get rejected until they are actually supported.

> 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c
> b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > index fd3071f55bd3..2867a2581a36 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct
> ptp_clock_info *ptp,
> >
> >  	switch (rq->type) {
> >  	case PTP_CLK_REQ_EXTTS:
> > +		/* Reject requests with unsupported flags */
> > +		if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE))
> > +			return -EOPNOTSUPP;
> 
> This HW always time stamps both edges, and that is not configurable.
> Here you reject PTP_FALLING_EDGE, and that is clearly wrong.  If the
> driver had been really picky (my fault I guess), it should have always
> insisted on (PTP_RISING_EDGE | PTP_FALLING_EDGE) being set together.
> But it is too late to enforce that now, because it could break user
> space programs.

Yes.

> 
> I do agree with the sentiment of checking the flags at the driver
> level, but this needs to be done case by case, with the drivers'
> author's input.

Sure. I think the best immediate approach is to make sure all drivers reject any *new* flags, and each driver can decide whether they should reject rising, falling, etc.

> 
> (The req.perout.flags can be done unconditionally in all drivers,
> since there were never any valid flags, but req.extts.flags needs
> careful attention.)
> 

Right.

> Thanks,
> Richard