Re: [PATCH net 06/10] iavf: prevent accidental free of filter structure

[Stefan Assmann <sassmann@redhat.com>]

On 2021-11-16 20:18, Keller, Jacob E wrote:
> On 11/15/2021 11:24 PM, Stefan Assmann wrote:
> > On 2021-11-15 15:59, Tony Nguyen wrote:
> >> From: Jacob Keller <jacob.e.keller@intel.com>
> >>
> >> In iavf_config_clsflower, the filter structure could be accidentally
> >> released at the end, if iavf_parse_cls_flower or iavf_handle_tclass ever
> >> return a non-zero but positive value.
> >>
> >> In this case, the function continues through to the end, and will call
> >> kfree() on the filter structure even though it has been added to the
> >> linked list.
> >>
> >> This can actually happen because iavf_parse_cls_flower will return
> >> a positive IAVF_ERR_CONFIG value instead of the traditional negative
> >> error codes.
> > 
> > Hi Jacob,
> > 
> > where exactly does this happen?
> > Looking at iavf_parse_cls_flower() I see all returns of IAVF_ERR_CONFIG
> > as "return IAVF_ERR_CONFIG;" while IAVF_ERR_CONFIG is defined as
> >         IAVF_ERR_CONFIG                         = -4,
> > 
> > I'm not opposed to this change, just wondering what's going on.
> > 
> >   Stefan
> > 
> 
> Heh.
> 
> I don't have memory of the full context for the original work. We've
> been going through and trying to pull in fixes that we've done for our
> out-of-tree driver and get everything upstream.
> 
> At first I thought this might be because of some history where these
> values used to be positive in the out-of-tree history at some point...
> But I think this wasn't true. It is possible that some other flow
> accidentally sends a positive value, but I've long since lost memory of
> if I had an example of that. You're correct that IAVF_ERR_CONFIG is (and
> has been in both upstream and out-of-tree code since its inception)
> negative.
> 
> I don't think this change is harmful, but I think you're right in
> pointing out the description isn't really valid.
> 
> I'm happy to re-write this commit message for clarity.
> 
> I do think switching to "if (err)" is more idiomatic and the correct
> thing to do.

Great feedback thanks, I totally agree.

  Stefan