Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Sowmini Varadhan]

On (10/17/16 12:49), Alexander Duyck wrote:
> >> > /* Currently only IPv4/IPv6 with TCP is supported */
> >> > switch (hdr.ipv4->version) {
> >> > case IPVERSION:
> >> > /* access ihl as u8 to avoid unaligned access on ia64 */
> >> > hlen = (hdr.network[0] & 0x0F) << 2;
> >> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> >> > +   sizeof(struct tcphdr))
> >> > +   return;
> >> > l4_proto = hdr.ipv4->protocol;
> >> > break;
> >> > case 6:
> >> > hlen = hdr.network - skb->data;
> >> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> >> > +   sizeof(struct tcphdr))
> >> > +   return;
> >> > l4_proto = ipv6_find_hdr(skb, , IPPROTO_TCP, NULL, 
> >> > NULL);
> >> > hlen -= hdr.network - skb->data;
> >> > break;
   :
> >> So you probably need to add a check for "skb_tail_pointer(skb) <
> >> (hdr.network + hlen + 20)".
> >
> > But isnt that the same thing as the checks before l4_proto computation 
> > above?
> 
> Sort of.  The problem is IPv6 can include extension headers and that
> can totally mess with us.  So we need to do one more check to verify
> that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
> + hlenl.

Yes, you are right. So given that I already check that I have
at least 40 bytes past the network header, and ipv6_find_hdr
will pull up exthdrs as needed, my checks are not needed, and the
real ones should happen after we come out of that switch().

--Sowmini

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Alexander Duyck]

On Mon, Oct 17, 2016 at 12:18 PM, Sowmini Varadhan
 wrote:
> On (10/17/16 11:15), Alexander Duyck wrote:
>> I would say you probably only need the first check here for skb->data
>> and could probably skip the second part.  You will be testing for
>> skb_tail_pointer in all the other tests you added so this check is
>> redundant anyway.
>>
>> Also you might want to go through and wrap these with unlikely() since
>> most of these are exception cases.
>
> Ok.. v3 will have this.
>
>> > /* Currently only IPv4/IPv6 with TCP is supported */
>> > switch (hdr.ipv4->version) {
>> > case IPVERSION:
>> > /* access ihl as u8 to avoid unaligned access on ia64 */
>> > hlen = (hdr.network[0] & 0x0F) << 2;
>> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +   sizeof(struct tcphdr))
>> > +   return;
>> > l4_proto = hdr.ipv4->protocol;
>> > break;
>> > case 6:
>> > hlen = hdr.network - skb->data;
>> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +   sizeof(struct tcphdr))
>> > +   return;
>> > l4_proto = ipv6_find_hdr(skb, , IPPROTO_TCP, NULL, 
>> > NULL);
>> > hlen -= hdr.network - skb->data;
>> > break;
>>
>> I believe one more check is needed after this block to verify the TCP
>> header fields are present.
>>
>> So you probably need to add a check for "skb_tail_pointer(skb) <
>> (hdr.network + hlen + 20)".
>
> But isnt that the same thing as the checks before l4_proto computation above?

Sort of.  The problem is IPv6 can include extension headers and that
can totally mess with us.  So we need to do one more check to verify
that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
+ hlenl.

Thanks.

- Alex

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Sowmini Varadhan]

On (10/17/16 11:15), Alexander Duyck wrote:
> I would say you probably only need the first check here for skb->data
> and could probably skip the second part.  You will be testing for
> skb_tail_pointer in all the other tests you added so this check is
> redundant anyway.
> 
> Also you might want to go through and wrap these with unlikely() since
> most of these are exception cases.

Ok.. v3 will have this.

> > /* Currently only IPv4/IPv6 with TCP is supported */
> > switch (hdr.ipv4->version) {
> > case IPVERSION:
> > /* access ihl as u8 to avoid unaligned access on ia64 */
> > hlen = (hdr.network[0] & 0x0F) << 2;
> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> > +   sizeof(struct tcphdr))
> > +   return;
> > l4_proto = hdr.ipv4->protocol;
> > break;
> > case 6:
> > hlen = hdr.network - skb->data;
> > +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> > +   sizeof(struct tcphdr))
> > +   return;
> > l4_proto = ipv6_find_hdr(skb, , IPPROTO_TCP, NULL, 
> > NULL);
> > hlen -= hdr.network - skb->data;
> > break;
> 
> I believe one more check is needed after this block to verify the TCP
> header fields are present.
> 
> So you probably need to add a check for "skb_tail_pointer(skb) <
> (hdr.network + hlen + 20)".

But isnt that the same thing as the checks before l4_proto computation above?

--Sowmini

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Alexander Duyck]

On Mon, Oct 17, 2016 at 10:25 AM, Sowmini Varadhan
 wrote:
> For some Tx paths (e.g., tpacket_snd()), ixgbe_atr may be
> passed down an sk_buff that has the network and transport
> header in the paged data, so it needs to make sure these
> headers are available in the headlen bytes to calculate the
> l4_proto.
>
> This patch expect that network and transport headers are
> already available in the non-paged header dat.  The assumption
> is that the caller has set this up if l4_proto based Tx
> steering is desired.
>
> Signed-off-by: Sowmini Varadhan 
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   18 ++
>  1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index eceb47b..2cc1dae 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -54,6 +54,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "ixgbe.h"
>  #include "ixgbe_common.h"
> @@ -7651,11 +7652,16 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
> /* snag network header to get L4 type and address */
> skb = first->skb;
> hdr.network = skb_network_header(skb);
> +   if (hdr.network <= skb->data || hdr.network >= skb_tail_pointer(skb))
> +   return;

I would say you probably only need the first check here for skb->data
and could probably skip the second part.  You will be testing for
skb_tail_pointer in all the other tests you added so this check is
redundant anyway.

Also you might want to go through and wrap these with unlikely() since
most of these are exception cases.

> if (skb->encapsulation &&
> first->protocol == htons(ETH_P_IP) &&
> hdr.ipv4->protocol == IPPROTO_UDP) {
> struct ixgbe_adapter *adapter = q_vector->adapter;
>
> +   if (skb_tail_pointer(skb) < hdr.network + VXLAN_HEADROOM)
> +   return;
> +
> /* verify the port is recognized as VXLAN */
> if (adapter->vxlan_port &&
> udp_hdr(skb)->dest == adapter->vxlan_port)
> @@ -7666,15 +7672,27 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
> hdr.network = skb_inner_network_header(skb);
> }
>
> +   /* Make sure we have at least [minimum IPv4 header + TCP]
> +* or [IPv6 header] bytes
> +*/
> +   if (skb_tail_pointer(skb) < hdr.network + 40)
> +   return;
> +
> /* Currently only IPv4/IPv6 with TCP is supported */
> switch (hdr.ipv4->version) {
> case IPVERSION:
> /* access ihl as u8 to avoid unaligned access on ia64 */
> hlen = (hdr.network[0] & 0x0F) << 2;
> +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> +   sizeof(struct tcphdr))
> +   return;
> l4_proto = hdr.ipv4->protocol;
> break;
> case 6:
> hlen = hdr.network - skb->data;
> +   if (skb_tail_pointer(skb) < hdr.network + hlen +
> +   sizeof(struct tcphdr))
> +   return;
> l4_proto = ipv6_find_hdr(skb, , IPPROTO_TCP, NULL, NULL);
> hlen -= hdr.network - skb->data;
> break;

I believe one more check is needed after this block to verify the TCP
header fields are present.

So you probably need to add a check for "skb_tail_pointer(skb) <
(hdr.network + hlen + 20)".

Thanks.

- Alex