Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one was allocated

[Leon Romanovsky <leon@kernel.org>]

On Mon, Nov 07, 2022 at 01:55:58PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Leon Romanovsky <leon@kernel.org>
> >Sent: Monday, November 7, 2022 2:03 AM
> >To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> >Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >edumazet@google.com; Kees Cook <keescook@chromium.org>;
> >netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>;
> >intel-wired-lan@lists.osuosl.org; Ruhl, Michael J <michael.j.ruhl@intel.com>;
> >Keller, Jacob E <jacob.e.keller@intel.com>; G, GurucharanX
> ><gurucharanx.g@intel.com>
> >Subject: Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one
> >was allocated
> >
> >On Fri, Nov 04, 2022 at 01:54:13PM -0700, Tony Nguyen wrote:
> >> From: Kees Cook <keescook@chromium.org>
> >>
> >> Avoid potential use-after-free condition under memory pressure. If the
> >> kzalloc() fails, q_vector will be freed but left in the original
> >> adapter->q_vector[v_idx] array position.
> >>
> >> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: Eric Dumazet <edumazet@google.com>
> >> Cc: Jakub Kicinski <kuba@kernel.org>
> >> Cc: Paolo Abeni <pabeni@redhat.com>
> >> Cc: intel-wired-lan@lists.osuosl.org
> >> Cc: netdev@vger.kernel.org
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> >> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker
> >at Intel)
> >
> >You should use first and last names here.
> >
> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> >b/drivers/net/ethernet/intel/igb/igb_main.c
> >> index d6c1c2e66f26..c2bb658198bf 100644
> >> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> >> @@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter
> >*adapter,
> >>  	if (!q_vector) {
> >>  		q_vector = kzalloc(size, GFP_KERNEL);
> >>  	} else if (size > ksize(q_vector)) {
> >> -		kfree_rcu(q_vector, rcu);
> >> -		q_vector = kzalloc(size, GFP_KERNEL);
> >> +		struct igb_q_vector *new_q_vector;
> >> +
> >> +		new_q_vector = kzalloc(size, GFP_KERNEL);
> >> +		if (new_q_vector)
> >> +			kfree_rcu(q_vector, rcu);
> >> +		q_vector = new_q_vector;
> >
> >I wonder if this is correct.
> >1. if new_q_vector is NULL, you will overwrite q_vector without releasing it.
> >2. kfree_rcu() doesn't immediately release memory, but after grace
> >period, but here you are overwriting the pointer which is not release
> >yet.
> 
> The actual pointer is: adapter->q_vector[v_idx]
> 
> q_vector is just a convenience pointer.
> 
> If the allocation succeeds, the q_vector[v_idx] will be replaced (later in the code).
> 
> If the allocation fails, this is not being freed.  The original code freed the adapter
> pointer but didn't not remove the pointer.
> 
> If q_vector is NULL,  (i.e. the allocation failed), the function exits, but the original
> pointer is left in place.
> 
> I think this logic is correct.
> 
> The error path leaves the original allocation in place.  If this is incorrect behavior,
> a different change would be:
> 
> 	q_vector = adapter->q_vector[v_idx];
> 	adapter->q_vector[v_idx] = NULL;
> 	... the original code...
> 
> But I am not sure if that is what is desired?

I understand the issue what you are trying to solve, I just don't
understand your RCU code. I would expect calls to rcu_dereference()
in order to get q_vector and rcu_assign_pointer() to clear
adapter->q_vector[v_idx], but igb has none.

Thanks