[Swan-dev] [PATCH libreswan v2] netlink: Silence negative shift coverity false warning

[Aviv Heller]

-----Original message-----
> From: Paul Wouters
> Sent: Wednesday, October 4 2017, 9:21 pm
> To: Antony Antony
> Cc: Aviv Heller; Libreswan Development List
> Subject: Re: [Swan-dev] [PATCH libreswan v2] netlink: Silence negative shift coverity false warning
> 
> I was also waiting on this patch to ensure it goes into RHEL. 
> 
> Sent from my iPhone
> 
> > On Oct 4, 2017, at 14:14, Antony Antony <antony at phenome.org> wrote:
> > 
> > Hi Aviv,
> > 
> > On Sun, Sep 24, 2017 at 05:05:42PM +0000, Aviv Heller wrote:
> >>> coverity-detected anomalies are sometimes subtle.  So I looked at this
> >>> code and found a couple of bugs.  I also did some tidying.  But no
> >>> testing!
> >>> 
> >>> Aviv, Antony: please have a look at commit
> >>> f7aaa80198851ef71da4be8dc54e816f7064e29a
> >> 
> >> Hi Hugh, Antony,
> >> 
> >> Hugh, thanks for bringing this up, there are indeed a few bugs in the code.
> >> 
> >>> Note: % 31 is almost never correct.  I think that in this code, + 31 is 
> >>> also wrong.
> >> 
> >> The (% 31) is indeed incorrect and should be (% 32).
> >> Also, another bug fixed in your patch is the strneq() being compared to 0 instead of 1.
> >> 
> >> Digging a little into ethtool ioctl code, the size passed (and correspondingly the array length) in cmd->size to ETHTOOL_GFEATURES should be the total number of blocks (dwords) possible for all options, or basically, (sset_len + 31) / 32 [put in the numbers and see that this calculation is correct].
> >> Failing to satisfy this will result in failure in case our ESP bit is no longer located in the last dword.
> >> 
> >>> Could someone with real hardware test this code?
> >> It works, with the limitation described in '2' above.
> > 
> > is nic-offload=auto broken now in master? 
> > 
> > If + 31 is correct f7aaa80198851 need fixing? 
> > 
> > -       unsigned block = netlink_esp_hw_offload / 32;
> > +       unsigned block = (netlink_esp_hw_offload + 31) / 32;
> > 
> >>> I cannot see that the old code would use NIC offload since the test for 
> >>> the feature seemed to be backwards.
> >>> 
> >>> Of course I could be backwards.
> >> No, it indeed didn't work.
> >> 
> >> I also created a patch with all these fixes based on the original code, in case it is needed.
> > 
> > Did you send a patch? I do not have anything else other than the old one,
> > [Swan-dev] [PATCH libreswan v2], Sep 04, 2017
> > 

Hi Hugh, Antony, Paul,

I'm sorry for the delayed reply, we had a busy period here.

I didn't send the patch - it was now rebased on top of latest master, and will be sent momentarily.

Hugh, the issue in your code is that when querying for ETHTOOL_GFEATURES, the total number of blocks given and allocated should be the maximum number considering all features, not just the offload bit position.
This means that once a new feature block will be added, the ioctl will fail with -EFAULT (see http://elixir.free-electrons.com/linux/latest/source/net/core/ethtool.c#L131).

Thanks,
Aviv


> > regards,
> > -antony
> > _______________________________________________
> > Swan-dev mailing list
> > Swan-dev at lists.libreswan.org
> > https://lists.libreswan.org/mailman/listinfo/swan-dev
> 
>