[ovs-dev,v2] flow: fix incorrect padding length checking and combine branch in ipv6_sanity_check

The padding length is (packet size - ipv6 header length - ipv6 plen).  This
patch fixes incorrect ipv6 size checking and improves it by combining branch.

Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Signed-off-by: Yanqin Wei <Yanqin.Wei@arm.com>
---
 lib/flow.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

On Thu, Aug 22, 2019 at 06:09:34PM +0800, Yanqin Wei wrote:
> The padding length is (packet size - ipv6 header length - ipv6 plen).  This
> patch fixes incorrect ipv6 size checking and improves it by combining branch.
> 
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Signed-off-by: Yanqin Wei <Yanqin.Wei@arm.com>
> ---
>  lib/flow.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index e5b554b..1b21f51 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -688,18 +688,16 @@ ipv4_get_nw_frag(const struct ip_header *nh)
>  static inline bool
>  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
>  {
> -    uint16_t plen;
> +    int pad_len;
>  
>      if (OVS_UNLIKELY(size < sizeof *nh)) {
>          return false;
>      }
>  
> -    plen = ntohs(nh->ip6_plen);
> -    if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> -        return false;
> -    }
> +    pad_len = size - IPV6_HEADER_LEN - ntohs(nh->ip6_plen);

The types in the above calculation worry me.  Writing the type of each
term:

    int = size_t - int - uint16_t

The most likely type of the right side of the expression is size_t.
Assigning this to an 'int' might do "the right thing" for negative
values, but it's risky--especially since size_t and int might be
different widths.  I think it would be safer to cast the first and third
terms to int, e.g.:

    pad_len = (int) size - IPV6_HEADER_LEN - (int) ntohs(nh->ip6_plen);

>      /* Jumbo Payload option not supported yet. */
> -    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> +    if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
>          return false;
>      }

Thanks,

Ben.

Hi Ben,

Thanks for the comments. I am sorry not to notice the risk of calculation with different type . 
The original reason to use 'int' for size checking is that compiler can combine two conditions into one and save a branch instruction here,  because "negative value" always more than UINT8_MAX.  
> +    if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {

But now I realized it introduces the risk and not clean for C code even if we use type cast here. So I will remove this performance improvement and only keep the bug fix for padding length calculation in this patch. What do you think of it?

Best Regards,
Wei Yanqin

-----Original Message-----
From: Ben Pfaff <blp@ovn.org> 
Sent: Thursday, August 29, 2019 5:58 AM
To: Yanqin Wei (Arm Technology China) <Yanqin.Wei@arm.com>
Cc: dev@openvswitch.org; nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
Subject: Re: [ovs-dev] [PATCH v2] flow: fix incorrect padding length checking and combine branch in ipv6_sanity_check

On Thu, Aug 22, 2019 at 06:09:34PM +0800, Yanqin Wei wrote:
> The padding length is (packet size - ipv6 header length - ipv6 plen).  
> This patch fixes incorrect ipv6 size checking and improves it by combining branch.
> 
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Signed-off-by: Yanqin Wei <Yanqin.Wei@arm.com>
> ---
>  lib/flow.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index e5b554b..1b21f51 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -688,18 +688,16 @@ ipv4_get_nw_frag(const struct ip_header *nh)  
> static inline bool  ipv6_sanity_check(const struct 
> ovs_16aligned_ip6_hdr *nh, size_t size)  {
> -    uint16_t plen;
> +    int pad_len;
>  
>      if (OVS_UNLIKELY(size < sizeof *nh)) {
>          return false;
>      }
>  
> -    plen = ntohs(nh->ip6_plen);
> -    if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> -        return false;
> -    }
> +    pad_len = size - IPV6_HEADER_LEN - ntohs(nh->ip6_plen);

The types in the above calculation worry me.  Writing the type of each
term:

    int = size_t - int - uint16_t

The most likely type of the right side of the expression is size_t.
Assigning this to an 'int' might do "the right thing" for negative values, but it's risky--especially since size_t and int might be different widths.  I think it would be safer to cast the first and third terms to int, e.g.:

    pad_len = (int) size - IPV6_HEADER_LEN - (int) ntohs(nh->ip6_plen);

>      /* Jumbo Payload option not supported yet. */
> -    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> +    if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
>          return false;
>      }

Thanks,

Ben.

The bug fix should definitely go in.  Maybe the performance improvement
could be separated.

On Thu, Aug 29, 2019 at 08:21:22AM +0000, Yanqin Wei (Arm Technology China) wrote:
> Hi Ben,
> 
> Thanks for the comments. I am sorry not to notice the risk of calculation with different type . 
> The original reason to use 'int' for size checking is that compiler can combine two conditions into one and save a branch instruction here,  because "negative value" always more than UINT8_MAX.  
> > +    if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
> 
> But now I realized it introduces the risk and not clean for C code even if we use type cast here. So I will remove this performance improvement and only keep the bug fix for padding length calculation in this patch. What do you think of it?
> 
> Best Regards,
> Wei Yanqin
> 
> -----Original Message-----
> From: Ben Pfaff <blp@ovn.org> 
> Sent: Thursday, August 29, 2019 5:58 AM
> To: Yanqin Wei (Arm Technology China) <Yanqin.Wei@arm.com>
> Cc: dev@openvswitch.org; nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Subject: Re: [ovs-dev] [PATCH v2] flow: fix incorrect padding length checking and combine branch in ipv6_sanity_check
> 
> On Thu, Aug 22, 2019 at 06:09:34PM +0800, Yanqin Wei wrote:
> > The padding length is (packet size - ipv6 header length - ipv6 plen).  
> > This patch fixes incorrect ipv6 size checking and improves it by combining branch.
> > 
> > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > Signed-off-by: Yanqin Wei <Yanqin.Wei@arm.com>
> > ---
> >  lib/flow.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/flow.c b/lib/flow.c
> > index e5b554b..1b21f51 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -688,18 +688,16 @@ ipv4_get_nw_frag(const struct ip_header *nh)  
> > static inline bool  ipv6_sanity_check(const struct 
> > ovs_16aligned_ip6_hdr *nh, size_t size)  {
> > -    uint16_t plen;
> > +    int pad_len;
> >  
> >      if (OVS_UNLIKELY(size < sizeof *nh)) {
> >          return false;
> >      }
> >  
> > -    plen = ntohs(nh->ip6_plen);
> > -    if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> > -        return false;
> > -    }
> > +    pad_len = size - IPV6_HEADER_LEN - ntohs(nh->ip6_plen);
> 
> The types in the above calculation worry me.  Writing the type of each
> term:
> 
>     int = size_t - int - uint16_t
> 
> The most likely type of the right side of the expression is size_t.
> Assigning this to an 'int' might do "the right thing" for negative values, but it's risky--especially since size_t and int might be different widths.  I think it would be safer to cast the first and third terms to int, e.g.:
> 
>     pad_len = (int) size - IPV6_HEADER_LEN - (int) ntohs(nh->ip6_plen);
> 
> >      /* Jumbo Payload option not supported yet. */
> > -    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> > +    if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
> >          return false;
> >      }
> 
> Thanks,
> 
> Ben.