netdev - Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200617090352.GA3113@salvia>
Date:   Wed, 17 Jun 2020 11:03:52 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     wenxu <wenxu@...oud.cn>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, vladbu@...lanox.com
Subject: Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for
 flow_block_cb

On Wed, Jun 17, 2020 at 10:42:19AM +0800, wenxu wrote:
[...]
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> index ef7f6bc..042c285 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> @@ -1918,6 +1918,7 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
> >>  
> >>  		flow_block_cb_add(block_cb, f);
> >>  		list_add_tail(&block_cb->driver_list, &bnxt_block_cb_list);
> >> +		block_cb->indr.cb_priv = bp;
> > cb_indent ?
> >
> > Why are you splitting the fix in multiple patches? This makes it
> > harder to review.
> >
> > I think patch 1/4, 2/4 and 4/4 belong to the same logical change?
> > Collapse them.
> 
> I think patch 1/4, 2/4, 4/4 are different bugs， Although they are all in the new indirect
> flow_block infrastructure.
> 
> patch1 fix the miss cleanup for flow_block_cb of flowtable
> 
> patch2 fix the incorrect check the cb_priv of flow_block_cb
> 
> patch4 fix the miss driver_list del in the cleanup callback
> 
> So maybe make them alone is better?

Maybe.

I'm attaching the collapsed patch for preview.

        10 files changed, 21 insertions(+), 15 deletions(-)

This one single collapsed patch is small.

And it looks like it is part of the same logic fix for the .cleanup
path? You had to update three aspects to make the cleanup path work.

3/4 is a different code path, leaving it standalone makes sense to me,
on top of this one probably.

This is just a proposal.

Thank you.

View attachment "0001-net-flow_offload-fix-flow_indr_dev_unregister-path.patch" of type "text/x-diff" (9822 bytes)