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 <b...@ovn.org>
> Sent: Thursday, August 29, 2019 5:58 AM
> To: Yanqin Wei (Arm Technology China) <yanqin....@arm.com>
> Cc: d...@openvswitch.org; nd <n...@arm.com>; Gavin Hu (Arm Technology China)
> <gavin...@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...@arm.com>
> > Signed-off-by: Yanqin Wei <yanqin....@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
