-----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 <[email protected]> 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 > > [email protected] > > https://lists.libreswan.org/mailman/listinfo/swan-dev > > _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
