netdev - Re: [PATCH RFC 3/3] mlx4: Call skb_csum_offload

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]

Date:	Wed, 7 Oct 2015 11:07:49 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Or Gerlitz <ogerlitz@...lanox.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Kernel Team <kernel-team@...com>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH RFC 3/3] mlx4: Call skb_csum_offload_check to check offloadability

On Wed, Oct 7, 2015 at 8:41 AM, Or Gerlitz <ogerlitz@...lanox.com> wrote:
> On 10/6/2015 2:39 AM, Tom Herbert wrote:
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 494e776..f364ffd 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -702,6 +702,14 @@ static void mlx4_bf_copy(void __iomem *dst, const
>> void *src,
>>         __iowrite64_copy(dst, src, bytecnt / 8);
>>   }
>>   +static const struct skb_csum_offl_spec csum_offl_spec = {
>> +       .ipv4_okay = 1,
>> +       .ipv6_okay = 1,
>> +       .encap_okay = 1,
>> +       .tcp_okay = 1,
>> +       .udp_okay = 1,
>> +};
>> +
>>   netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>>         struct skb_shared_info *shinfo = skb_shinfo(skb);
>> @@ -727,6 +735,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>>         bool stop_queue;
>>         bool inline_ok;
>>         u32 ring_cons;
>> +       struct skb_csum_offl_params offl_params;
>>         if (!priv->port_up)
>>                 goto tx_drop;
>> @@ -853,8 +862,9 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>>         /* Prepare ctrl segement apart opcode+ownership, which depends on
>>          * whether LSO is used */
>>         tx_desc->ctrl.srcrb_flags = priv->ctrl_flags;
>> -       if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
>> -               if (!skb->encapsulation)
>> +       if (skb_csum_offload_chk(skb, &csum_offl_spec,
>> +                                &offl_params, true)) {
>> +               if (!offl_params.encapped_csum)
>>                         tx_desc->ctrl.srcrb_flags |=
>> cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |
>>
>> MLX4_WQE_CTRL_TCP_UDP_CSUM);
>>                 else
>> @@ -912,9 +922,9 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>>                 build_inline_wqe(tx_desc, skb, shinfo, real_size,
>> &vlan_tag,
>>                                  tx_ind, fragptr);
>>   -     if (skb->encapsulation) {
>> -               struct iphdr *ipv4 = (struct iphdr
>> *)skb_inner_network_header(skb);
>> -               if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol ==
>> IPPROTO_UDP)
>> +       if (!offl_params.encapped_csum) {
>> +               if (offl_params.ip_proto == IPPROTO_TCP ||
>> +                   offl_params.ip_proto == IPPROTO_UDP)
>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP |
>> MLX4_WQE_CTRL_ILP);
>>                 else
>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);
>> -- 2.4.6
>
> few quick comments
>
> not all mlx4 devices support offloading checksum for tunneled traffic, only
> ConnectX3-pro -- this translates for the mlx4 EN driver
> logic that determines that on startup, e.g only when
> mdev->dev->caps.tunnel_offload_mode == MLX4_TUNNEL_OFFLOAD_MODE_VXLAN
> we set hw_enc_features and friends -- in that respect, I wasn't sure if it's
> okay to blindly set  .encap_oka = 1 here.
>
Or, I would only give the mlnx support as an example. I think driver
owners would need to implement the specification for the their
devices.

> Another constraint, is that when the device does support (say) TCP TX
> checksum offload for the inner packet they don't support UDP
> checksum offload for the outer packet.

We can add such things to the specification. One value I see in having
a common structure to describe the checksum capabilities is that
becomes a way to clearly document what is (and is not) supported by
devices.

btw, I don't quite understand your example. If a device does not
support UDP checksum there is a flag for that in the specification. If
the stack sends a TCP packet encapsulated in a UDP packet with UDP
checksum enabled, it will try to offload the UDP checksum and not the
TCP one. There is currently no interface to offload two checksums in
the same packet (in non-GRO at least) and with things like RCO we
probably will never need that anyway.

Tom
--
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