netdev - RE: [net-next 1/4] i40e: add support for PTP external synchronization clock

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]

Message-ID: <MN2PR11MB3565041DE307A4FA48DCC5D0F3E10@MN2PR11MB3565.namprd11.prod.outlook.com>
Date:   Wed, 18 Nov 2020 16:42:46 +0000
From:   "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
        "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>,
        Andrew Bowers <andrewx.bowers@...el.com>,
        Richard Cochran <richardcochran@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: RE: [net-next 1/4] i40e: add support for PTP external synchronization
 clock

>-----Original Message-----
>From: Jakub Kicinski <kuba@...nel.org>
>Sent: Tuesday, November 17, 2020 2:08 AM
>To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
>Cc: davem@...emloft.net; Kwapulinski, Piotr <piotr.kwapulinski@...el.com>; netdev@...r.kernel.org; sassmann@...hat.com; Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@...el.com>; Andrew Bowers <andrewx.bowers@...el.com>; Richard Cochran <richardcochran@...il.com>; Vladimir Oltean <olteanv@...il.com>
>Subject: Re: [net-next 1/4] i40e: add support for PTP external synchronization clock
>
>On Fri, 13 Nov 2020 16:10:54 -0800 Tony Nguyen wrote:
>> From: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>>
>> Add support for external synchronization clock via GPIOs.
>> 1PPS signals are handled via the dedicated 3 GPIOs: SDP3_2,
>> SDP3_3 and GPIO_4.
>> Previously it was not possible to use the external PTP synchronization
>> clock.
>
>Please _always_ CC Richard on PTP changes.

Sure

>
>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
>> b/drivers/net/ethernet/intel/i40e/i40e.h
>> index 537300e762f0..8f5eecbff3d6 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
>> @@ -196,6 +196,11 @@ enum i40e_fd_stat_idx {  #define
>> I40E_FD_ATR_TUNNEL_STAT_IDX(pf_id) \
>>                      (I40E_FD_STAT_PF_IDX(pf_id) + I40E_FD_STAT_ATR_TUNNEL)
>>
>> +/* get PTP pins for ioctl */
>> +#define SIOCGPINS   (SIOCDEVPRIVATE + 0)
>> +/* set PTP pins for ioctl */
>> +#define SIOCSPINS   (SIOCDEVPRIVATE + 1)
>
>This is unexpected.. is it really normal to declare private device IOCTLs to configure PPS pins? Or are you just exposing this so you're able to play with GPIOs from user space?

Right, this should not go upstream.

>
>>  /* The following structure contains the data parsed from the user-defined
>>   * field of the ethtool_rx_flow_spec structure.
>>   */
>> @@ -344,7 +349,6 @@ struct i40e_ddp_old_profile_list {
>>                                           I40E_FLEX_SET_FSIZE(fsize) | \
>>                                           I40E_FLEX_SET_SRC_WORD(src))
>>
>> -
>
>Please move all the empty line removal to a separate patch.
>
>>  #define I40E_MAX_FLEX_SRC_OFFSET 0x1F
>>
>>  /* macros related to GLQF_ORT */
>
>> @@ -2692,7 +2692,15 @@ int i40e_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>>      case SIOCGHWTSTAMP:
>>              return i40e_ptp_get_ts_config(pf, ifr);
>>      case SIOCSHWTSTAMP:
>> +            if (!capable(CAP_SYS_ADMIN))
>> +                    return -EACCES;
>
>If you needed this, this should be a fix for net. But you don't, core checks it.
>
>>              return i40e_ptp_set_ts_config(pf, ifr);
>> +    case SIOCSPINS:
>> +            if (!capable(CAP_SYS_ADMIN))
>> +                    return -EACCES;
>> +            return i40e_ptp_set_pins_ioctl(pf, ifr);
>> +    case SIOCGPINS:
>> +            return i40e_ptp_get_pins(pf, ifr);
>>      default:
>>              return -EOPNOTSUPP;
>>      }
>

I'll provide and updated patch.
Thank you for review.