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

On 11/15/2021 11:24 PM, Stefan Assmann wrote:

On 2021-11-15 15:59, Tony Nguyen wrote:

quoted

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.

Thanks,
Jake

quoted

Fix this by ensuring that the kfree() check and error checks are
similar. Use the more idiomatic "if (err)" to catch all non-zero error
codes.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 76c4ca0f055e..9c68c8628512 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c

@@ -3108,11 +3108,11 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
 	/* start out with flow type and eth type IPv4 to begin with */
 	filter->f.flow_type = VIRTCHNL_TCP_V4_FLOW;
 	err = iavf_parse_cls_flower(adapter, cls_flower, filter);
-	if (err < 0)
+	if (err)
 		goto err;
 
 	err = iavf_handle_tclass(adapter, tc, filter);
-	if (err < 0)
+	if (err)
 		goto err;
 
 	/* add filter to the list */

-- 
2.31.1