netdev - RE: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO

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: <VI1PR05MB35202FBCCC9C4B06F5C9148FBF3C0@VI1PR05MB3520.eurprd05.prod.outlook.com>
Date:   Mon, 4 Dec 2017 22:15:59 +0000
From:   Yuval Mintz <yuvalm@...lanox.com>
To:     Michael Chan <michael.chan@...adcom.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        "everest-linux-l2@...ium.com" <everest-linux-l2@...ium.com>
Subject: RE: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
> GRO.  With this flag, we can now independently turn on or off hardware
> GRO when GRO is on.  Hardware GRO guarantees that packets can be
> re-segmented by TSO/GSO to reconstruct the original packet stream.
> 
> Cc: Ariel Elior <Ariel.Elior@...ium.com>
> Cc: everest-linux-l2@...ium.com
> Signed-off-by: Michael Chan <michael.chan@...adcom.com>
> ---
>  Documentation/networking/netdev-features.txt |  7 +++++++
>  include/linux/netdev_features.h              |  5 ++++-
>  net/core/dev.c                               | 13 +++++++++++++
>  net/core/ethtool.c                           |  1 +
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/netdev-features.txt
> b/Documentation/networking/netdev-features.txt
> index 7413eb0..d76d332 100644
> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,10 @@ This requests that the NIC receive all possible
> frames, including errored
>  frames (such as bad FCS, etc).  This can be helpful when sniffing a link with
>  bad packets on it.  Some NICs may receive more packets if also put into
> normal
>  PROMISC mode.
> +
> +*  rx-gro-hw
> +
> +This requests that the NIC enables Hardware GRO (generic receive offload).
> +Hardware GRO is basically the exact reverse of TSO, and is generally
> +stricter than Hardware LRO.  A packet stream merged by Hardware GRO
> must
> +be re-segmentable by GSO or TSO back to the exact packet stream.
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index dc8b489..d18ef6f 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -77,6 +77,8 @@ enum {
>  	NETIF_F_HW_ESP_TX_CSUM_BIT,	/* ESP with TX checksum
> offload */
>  	NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP
> tunnels */
> 
> +	NETIF_F_GRO_HW_BIT,		/* Hardware Generic receive
> offload */
> +
>  	/*
>  	 * Add your fresh new feature above and remember to update
>  	 * netdev_features_strings[] in net/core/ethtool.c and maybe
> @@ -96,6 +98,7 @@ enum {
>  #define NETIF_F_FRAGLIST	__NETIF_F(FRAGLIST)
>  #define NETIF_F_FSO		__NETIF_F(FSO)
>  #define NETIF_F_GRO		__NETIF_F(GRO)
> +#define NETIF_F_GRO_HW		__NETIF_F(GRO_HW)
>  #define NETIF_F_GSO		__NETIF_F(GSO)
>  #define NETIF_F_GSO_ROBUST	__NETIF_F(GSO_ROBUST)
>  #define NETIF_F_HIGHDMA		__NETIF_F(HIGHDMA)
> @@ -193,7 +196,7 @@ enum {
>   * If upper/master device has these features disabled, they must be disabled
>   * on all lower/slave devices as well.
>   */
> -#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
> +#define NETIF_F_UPPER_DISABLES	(NETIF_F_LRO | NETIF_F_GRO_HW)
> 
>  /* changeable features with no special hardware requirements */
>  #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 30b5fe3..09c2ad0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7392,6 +7392,19 @@ static netdev_features_t
> netdev_fix_features(struct net_device *dev,
>  		features &= ~dev->gso_partial_features;
>  	}
> 
> +	if (features & NETIF_F_GRO_HW) {
> +		/* Hardware GRO depends on GRO. */
> +		if (!(features & NETIF_F_GRO)) {

While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

> +			netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
> no GRO feature.\n");
> +			features &= ~NETIF_F_GRO_HW;
> +		}
> +		/* Hardware GRO and LRO are mutually exclusive. */
> +		if (features & NETIF_F_LRO) {
> +			netdev_dbg(dev, "Dropping NETIF_F_LRO since
> GRO_HW is set.\n");
> +			features &= ~NETIF_F_LRO;

Isn't this considered to be breaking an existing API?
After this, while NETIF_F_GRO_HW is published an application trying to
set NETIF_F_LRO and then query its state would discover it failed
[while previously it could have succeeded, such as for bnx2]
 
While I understand the need to make sure core doesn't enable
two competing aggregation offloads, why make GRO_HW > LRO?
I understand it's probably the better one, but until LRO gets deprecated
isn't it safer to do this limitation the opposite way?
I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

> +		}
> +	}
> +
>  	return features;
>  }
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf45..50a7920 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev,
> struct ethtool_ts_info *info)
>  	[NETIF_F_LLTX_BIT] =             "tx-lockless",
>  	[NETIF_F_NETNS_LOCAL_BIT] =      "netns-local",
>  	[NETIF_F_GRO_BIT] =              "rx-gro",
> +	[NETIF_F_GRO_HW_BIT] =           "rx-gro-hw",
>  	[NETIF_F_LRO_BIT] =              "rx-lro",
> 
>  	[NETIF_F_TSO_BIT] =              "tx-tcp-segmentation",
> --
> 1.8.3.1