| 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
| ||
|
Message-ID: <CA+mtBx9vYV5i9_u=XMmEgSV3Uj9SEJAZHrKhTgyGgBG9Ojs-dA@mail.gmail.com>
Date: Mon, 22 Dec 2014 17:28:29 -0800
From: Tom Herbert <therbert@...gle.com>
To: Jesse Gross <jesse@...ira.com>
Cc: David Miller <davem@...emloft.net>,
Linux Netdev List <netdev@...r.kernel.org>,
Joe Stringer <joestringer@...ira.com>,
Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
On Mon, Dec 22, 2014 at 8:03 AM, Jesse Gross <jesse@...ira.com> wrote:
> GSO isn't the only offload feature with restrictions that
> potentially can't be expressed with the current features mechanism.
> Checksum is another although it's a general issue that could in
> theory apply to anything. Even if it may be possible to
> implement these restrictions in other ways, it can result in
> duplicate code or inefficient per-packet behavior.
>
> This generalizes ndo_gso_check so that drivers can remove any
> features that don't make sense for a given packet, similar to
> netif_skb_features(). It also converts existing driver
> restrictions to the new format, completing the work that was
> done to support tunnel protocols since the issues apply to
> checksums as well.
>
It's a nice feature, but I really hope that this is not used for
checksums. We already have a sufficiently general interface for that
and checksum is already computed in drivers to work around HW bugs.
Acked-by: Tom Herbert <therbert@...gle.com>
> CC: Tom Herbert <therbert@...gle.com>
> CC: Joe Stringer <joestringer@...ira.com>
> CC: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Jesse Gross <jesse@...ira.com>
> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 8 ++++---
> drivers/net/ethernet/emulex/benet/be_main.c | 8 ++++---
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 10 +++++----
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 8 ++++---
> include/linux/netdevice.h | 20 +++++++++--------
> include/net/vxlan.h | 28 ++++++++++++++++++++----
> net/core/dev.c | 28 ++++++++++++++++--------
> 7 files changed, 75 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 9f5e387..72eef9f 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12553,9 +12553,11 @@ static int bnx2x_get_phys_port_id(struct net_device *netdev,
> return 0;
> }
>
> -static bool bnx2x_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t bnx2x_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
>
> static const struct net_device_ops bnx2x_netdev_ops = {
> @@ -12589,7 +12591,7 @@ static const struct net_device_ops bnx2x_netdev_ops = {
> #endif
> .ndo_get_phys_port_id = bnx2x_get_phys_port_id,
> .ndo_set_vf_link_state = bnx2x_set_vf_link_state,
> - .ndo_gso_check = bnx2x_gso_check,
> + .ndo_features_check = bnx2x_features_check,
> };
>
> static int bnx2x_set_coherency_mask(struct bnx2x *bp)
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 1960731..41a0a54 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -4459,9 +4459,11 @@ done:
> adapter->vxlan_port_count--;
> }
>
> -static bool be_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t be_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -4492,7 +4494,7 @@ static const struct net_device_ops be_netdev_ops = {
> #ifdef CONFIG_BE2NET_VXLAN
> .ndo_add_vxlan_port = be_add_vxlan_port,
> .ndo_del_vxlan_port = be_del_vxlan_port,
> - .ndo_gso_check = be_gso_check,
> + .ndo_features_check = be_features_check,
> #endif
> };
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 190cbd9..d0d6dc1 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2365,9 +2365,11 @@ static void mlx4_en_del_vxlan_port(struct net_device *dev,
> queue_work(priv->mdev->workqueue, &priv->vxlan_del_task);
> }
>
> -static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t mlx4_en_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -2400,7 +2402,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
> #ifdef CONFIG_MLX4_EN_VXLAN
> .ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
> .ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
> - .ndo_gso_check = mlx4_en_gso_check,
> + .ndo_features_check = mlx4_en_features_check,
> #endif
> };
>
> @@ -2434,7 +2436,7 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
> #ifdef CONFIG_MLX4_EN_VXLAN
> .ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
> .ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
> - .ndo_gso_check = mlx4_en_gso_check,
> + .ndo_features_check = mlx4_en_features_check,
> #endif
> };
>
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> index 1aa25b1..9929b97 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> @@ -505,9 +505,11 @@ static void qlcnic_del_vxlan_port(struct net_device *netdev,
> adapter->flags |= QLCNIC_DEL_VXLAN_PORT;
> }
>
> -static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t qlcnic_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -532,7 +534,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
> #ifdef CONFIG_QLCNIC_VXLAN
> .ndo_add_vxlan_port = qlcnic_add_vxlan_port,
> .ndo_del_vxlan_port = qlcnic_del_vxlan_port,
> - .ndo_gso_check = qlcnic_gso_check,
> + .ndo_features_check = qlcnic_features_check,
> #endif
> #ifdef CONFIG_NET_POLL_CONTROLLER
> .ndo_poll_controller = qlcnic_poll_controller,
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c31f74d..679e6e9 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1012,12 +1012,15 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * Callback to use for xmit over the accelerated station. This
> * is used in place of ndo_start_xmit on accelerated net
> * devices.
> - * bool (*ndo_gso_check) (struct sk_buff *skb,
> - * struct net_device *dev);
> + * netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
> + * struct net_device *dev
> + * netdev_features_t features);
> * Called by core transmit path to determine if device is capable of
> - * performing GSO on a packet. The device returns true if it is
> - * able to GSO the packet, false otherwise. If the return value is
> - * false the stack will do software GSO.
> + * performing offload operations on a given packet. This is to give
> + * the device an opportunity to implement any restrictions that cannot
> + * be otherwise expressed by feature flags. The check is called with
> + * the set of features that the stack has calculated and it returns
> + * those the driver believes to be appropriate.
> *
> * int (*ndo_switch_parent_id_get)(struct net_device *dev,
> * struct netdev_phys_item_id *psid);
> @@ -1178,8 +1181,9 @@ struct net_device_ops {
> struct net_device *dev,
> void *priv);
> int (*ndo_get_lock_subclass)(struct net_device *dev);
> - bool (*ndo_gso_check) (struct sk_buff *skb,
> - struct net_device *dev);
> + netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features);
> #ifdef CONFIG_NET_SWITCHDEV
> int (*ndo_switch_parent_id_get)(struct net_device *dev,
> struct netdev_phys_item_id *psid);
> @@ -3611,8 +3615,6 @@ static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
> netdev_features_t features)
> {
> return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
> - (dev->netdev_ops->ndo_gso_check &&
> - !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
> unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
> (skb->ip_summed != CHECKSUM_UNNECESSARY)));
> }
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 57cccd0..903461a 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -1,6 +1,9 @@
> #ifndef __NET_VXLAN_H
> #define __NET_VXLAN_H 1
>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/if_vlan.h>
> #include <linux/skbuff.h>
> #include <linux/netdevice.h>
> #include <linux/udp.h>
> @@ -51,16 +54,33 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
> __be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
> __be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
>
> -static inline bool vxlan_gso_check(struct sk_buff *skb)
> +static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
> + netdev_features_t features)
> {
> - if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> + u8 l4_hdr = 0;
> +
> + if (!skb->encapsulation)
> + return features;
> +
> + switch (vlan_get_protocol(skb)) {
> + case htons(ETH_P_IP):
> + l4_hdr = ip_hdr(skb)->protocol;
> + break;
> + case htons(ETH_P_IPV6):
> + l4_hdr = ipv6_hdr(skb)->nexthdr;
> + break;
> + default:
> + return features;;
> + }
> +
> + if ((l4_hdr == IPPROTO_UDP) &&
> (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> skb->inner_protocol != htons(ETH_P_TEB) ||
> (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
> sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
> - return false;
> + return features & ~(NETIF_F_ALL_CSUM | NETIF_F_GSO_MASK);
>
> - return true;
> + return features;
> }
>
> /* IP header + UDP + VXLAN + Ethernet header */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f411c28..fc13f72 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2562,7 +2562,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
>
> netdev_features_t netif_skb_features(struct sk_buff *skb)
> {
> - const struct net_device *dev = skb->dev;
> + struct net_device *dev = skb->dev;
> netdev_features_t features = dev->features;
> u16 gso_segs = skb_shinfo(skb)->gso_segs;
> __be16 protocol = skb->protocol;
> @@ -2570,11 +2570,19 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
> if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
> features &= ~NETIF_F_GSO_MASK;
>
> + /* If encapsulation offload request, verify we are testing
> + * hardware encapsulation features instead of standard
> + * features for the netdev
> + */
> + if (skb->encapsulation)
> + features = netdev_intersect_features(features,
> + dev->hw_enc_features);
> +
> if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD)) {
> struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
> protocol = veh->h_vlan_encapsulated_proto;
> } else if (!vlan_tx_tag_present(skb)) {
> - return harmonize_features(skb, features);
> + goto finalize;
> }
>
> features = netdev_intersect_features(features,
> @@ -2591,6 +2599,15 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
> NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_STAG_TX);
>
> +finalize:
> + if (dev->netdev_ops->ndo_features_check) {
> + netdev_features_t dev_features;
> +
> + dev_features = dev->netdev_ops->ndo_features_check(skb, dev,
> + features);
> + features = netdev_intersect_features(features, dev_features);
> + }
> +
> return harmonize_features(skb, features);
> }
> EXPORT_SYMBOL(netif_skb_features);
> @@ -2661,13 +2678,6 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
> if (unlikely(!skb))
> goto out_null;
>
> - /* If encapsulation offload request, verify we are testing
> - * hardware encapsulation features instead of standard
> - * features for the netdev
> - */
> - if (skb->encapsulation)
> - features &= dev->hw_enc_features;
> -
> if (netif_needs_gso(dev, skb, features)) {
> struct sk_buff *segs;
>
> --
> 1.9.1
>
--
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
Powered by blists - more mailing lists