netdev - Re: [PATCH v3] net: Allow no-cache copy from user on transmit

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: <AANLkTimJR+SxeoiZnyP3_vyuK7Z+w_2G2gtUp7pZGZiR@mail.gmail.com>
Date:	Thu, 31 Mar 2011 10:52:06 +0200
From:	Michał Mirosław <mirqus@...il.com>
To:	Tom Herbert <therbert@...gle.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v3] net: Allow no-cache copy from user on transmit

2011/3/31 Tom Herbert <therbert@...gle.com>:
> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
> on transmit to bypass data cache for a performance improvement.
> This functionality is configurable per device using ethtool.
[...]
> diff --git a/include/net/sock.h b/include/net/sock.h
> index da0534d..74ce586 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1401,6 +1401,11 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
>                if (err)
>                        return err;
>                skb->csum = csum_block_add(skb->csum, csum, skb->len);
> +       } else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
> +               if (!access_ok(VERIFY_READ, from, copy) ||
> +                   __copy_from_user_nocache(page_address(page) + off,
> +                                               from, copy))
> +                       return -EFAULT;
>        } else if (copy_from_user(page_address(page) + off, from, copy))
>                return -EFAULT;
>

BTW, the checks should take protocol into account. If, for example,
device has NOCACHE_COPY and CSUM_IPV4, but not CSUM_IPV6, then it will
take a hit for every IPv6 packet that needs checksumming in software.
Maybe this can be taken into account when sk_route_caps are set?

[...]
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1a6e9eb..59b6ce9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
>        int i;
>
>        features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
> -       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
> +       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
>
>        if (!bond->first_slave)
>                goto done;
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0b88eba..454abfd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5444,6 +5444,13 @@ int register_netdevice(struct net_device *dev)
>                dev->features &= ~NETIF_F_GSO;
>        }
>
> +       /* Turn no cache copy off if HW isn't doing checksum */
> +       if (!(dev->features & NETIF_F_ALL_CSUM) ||
> +           (dev->features & NETIF_F_NO_CSUM)) {
> +               dev->wanted_features &= ~NETIF_F_NOCACHE_COPY;
> +               dev->features &= ~NETIF_F_NOCACHE_COPY;
> +       }
> +
>        /* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
>         * vlan_dev_init() will do the dev->features check, so these features
>         * are enabled only if supported by underlying device.

Why NO_CSUM is disabling NOCACHE_COPY? You combine those features in
bonding code anyway.

You didn't add netdev_fix_features() checks, so NOCACHE_COPY might be
enabled via ethtool after device is registered even if driver doesn't
support checksum offloads.

Best Regards,
Michał Mirosław
--
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