[PATCH net-next 0/6][pull request] Intel Wired LAN Driver Updates 2022-11-04 (ixgbe, ixgbevf, igb)

[PATCH net-next 0/6][pull request] Intel Wired LAN Driver Updates 2022-11-04 (ixgbe, ixgbevf, igb)

[Tony Nguyen]

This series contains updates to ixgbe, ixgbevf, and igb drivers.

Daniel Willenson adjusts descriptor buffer limits to be based on what
hardware supports instead of using a generic, least common value for
ixgbe.

Ani removes local variable for ixgbe, instead returning conditional result
directly.

Yang Li removes unneeded semicolon for ixgbe.

Jan adds error messaging when VLAN errors are encountered for ixgbevf.

Kees Cook prevents a potential use after free condition and explicitly
rounds up q_vector allocations so that allocations can be correctly
compared to ksize() for igb.

The following are changes since commit 95ec6bce2a0bb7ec9c76fe5c2a9db3b9e62c950d:
  Merge branch 'net-ipa-more-endpoints'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 10GbE

Anirudh Venkataramanan (1):
  ixgbe: Remove local variable

Daniel Willenson (1):
  ixgbe: change MAX_RXD/MAX_TXD based on adapter type

Jan Sokolowski (1):
  ixgbevf: Add error messages on vlan error

Kees Cook (2):
  igb: Do not free q_vector unless new one was allocated
  igb: Proactively round up to kmalloc bucket size

Yang Li (1):
  ixgbe: Remove unneeded semicolon

 drivers/net/ethernet/intel/igb/igb_main.c     | 10 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      | 10 +++-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  | 53 +++++++++++++++----
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c  |  2 +-
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 17 ++++--
 5 files changed, 70 insertions(+), 22 deletions(-)

-- 
2.35.1

[PATCH net-next 1/6] ixgbe: change MAX_RXD/MAX_TXD based on adapter type

[Tony Nguyen]

From: Daniel Willenson <daniel@veobot.com>

Set the length limit for the receive descriptor buffer and transmit
descriptor buffer based on the controller type. The values used are called
out in the controller datasheets as a 'Note:' in the RDLEN and TDLEN
register descriptions.

This allows the user to use ethtool to allocate larger descriptor buffers
in the case where data is received or transmitted too quickly for the
driver to keep up.

Signed-off-by: Daniel Willenson <daniel@veobot.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      | 10 ++++-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  | 44 +++++++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 5369a97ff5ec..bc68b8f2176d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -39,7 +39,10 @@
 /* TX/RX descriptor defines */
 #define IXGBE_DEFAULT_TXD		    512
 #define IXGBE_DEFAULT_TX_WORK		    256
-#define IXGBE_MAX_TXD			   4096
+#define IXGBE_MAX_TXD_82598		   4096
+#define IXGBE_MAX_TXD_82599		   8192
+#define IXGBE_MAX_TXD_X540		   8192
+#define IXGBE_MAX_TXD_X550		  32768
 #define IXGBE_MIN_TXD			     64
 
 #if (PAGE_SIZE < 8192)
@@ -47,7 +50,10 @@
 #else
 #define IXGBE_DEFAULT_RXD		    128
 #endif
-#define IXGBE_MAX_RXD			   4096
+#define IXGBE_MAX_RXD_82598		   4096
+#define IXGBE_MAX_RXD_82599		   8192
+#define IXGBE_MAX_RXD_X540		   8192
+#define IXGBE_MAX_RXD_X550		  32768
 #define IXGBE_MIN_RXD			     64
 
 /* flow control */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index eda7188e8df4..d737e851a9e0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1117,6 +1117,42 @@ static void ixgbe_get_drvinfo(struct net_device *netdev,
 	drvinfo->n_priv_flags = IXGBE_PRIV_FLAGS_STR_LEN;
 }
 
+static u32 ixgbe_get_max_rxd(struct ixgbe_adapter *adapter)
+{
+	switch (adapter->hw.mac.type) {
+	case ixgbe_mac_82598EB:
+		return IXGBE_MAX_RXD_82598;
+	case ixgbe_mac_82599EB:
+		return IXGBE_MAX_RXD_82599;
+	case ixgbe_mac_X540:
+		return IXGBE_MAX_RXD_X540;
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+	case ixgbe_mac_x550em_a:
+		return IXGBE_MAX_RXD_X550;
+	default:
+		return IXGBE_MAX_RXD_82598;
+	}
+}
+
+static u32 ixgbe_get_max_txd(struct ixgbe_adapter *adapter)
+{
+	switch (adapter->hw.mac.type) {
+	case ixgbe_mac_82598EB:
+		return IXGBE_MAX_TXD_82598;
+	case ixgbe_mac_82599EB:
+		return IXGBE_MAX_TXD_82599;
+	case ixgbe_mac_X540:
+		return IXGBE_MAX_TXD_X540;
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+	case ixgbe_mac_x550em_a:
+		return IXGBE_MAX_TXD_X550;
+	default:
+		return IXGBE_MAX_TXD_82598;
+	}
+}
+
 static void ixgbe_get_ringparam(struct net_device *netdev,
 				struct ethtool_ringparam *ring,
 				struct kernel_ethtool_ringparam *kernel_ring,
@@ -1126,8 +1162,8 @@ static void ixgbe_get_ringparam(struct net_device *netdev,
 	struct ixgbe_ring *tx_ring = adapter->tx_ring[0];
 	struct ixgbe_ring *rx_ring = adapter->rx_ring[0];
 
-	ring->rx_max_pending = IXGBE_MAX_RXD;
-	ring->tx_max_pending = IXGBE_MAX_TXD;
+	ring->rx_max_pending = ixgbe_get_max_rxd(adapter);
+	ring->tx_max_pending = ixgbe_get_max_txd(adapter);
 	ring->rx_pending = rx_ring->count;
 	ring->tx_pending = tx_ring->count;
 }
@@ -1146,11 +1182,11 @@ static int ixgbe_set_ringparam(struct net_device *netdev,
 		return -EINVAL;
 
 	new_tx_count = clamp_t(u32, ring->tx_pending,
-			       IXGBE_MIN_TXD, IXGBE_MAX_TXD);
+			       IXGBE_MIN_TXD, ixgbe_get_max_txd(adapter));
 	new_tx_count = ALIGN(new_tx_count, IXGBE_REQ_TX_DESCRIPTOR_MULTIPLE);
 
 	new_rx_count = clamp_t(u32, ring->rx_pending,
-			       IXGBE_MIN_RXD, IXGBE_MAX_RXD);
+			       IXGBE_MIN_RXD, ixgbe_get_max_rxd(adapter));
 	new_rx_count = ALIGN(new_rx_count, IXGBE_REQ_RX_DESCRIPTOR_MULTIPLE);
 
 	if ((new_tx_count == adapter->tx_ring_count) &&
-- 
2.35.1

[PATCH net-next 2/6] ixgbe: Remove local variable

[Tony Nguyen]

From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>

Remove local variable "match" and directly return evaluated conditional
instead.

Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index d737e851a9e0..6cfc9dc16537 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1996,18 +1996,13 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
 				     unsigned int frame_size)
 {
 	unsigned char *data;
-	bool match = true;
 
 	frame_size >>= 1;
 
 	data = page_address(rx_buffer->page) + rx_buffer->page_offset;
 
-	if (data[3] != 0xFF ||
-	    data[frame_size + 10] != 0xBE ||
-	    data[frame_size + 12] != 0xAF)
-		match = false;
-
-	return match;
+	return data[3] == 0xFF && data[frame_size + 10] == 0xBE &&
+		data[frame_size + 12] == 0xAF;
 }
 
 static u16 ixgbe_clean_test_rings(struct ixgbe_ring *rx_ring,
-- 
2.35.1

[PATCH net-next 3/6] ixgbe: Remove unneeded semicolon

[Tony Nguyen]

From: Yang Li <yang.lee@linux.alibaba.com>

./drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c:1305:2-3: Unneeded semicolon

Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=2688
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index fd3f77a9e28d..0310af851086 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -1302,7 +1302,7 @@ static void ixgbe_ptp_init_systime(struct ixgbe_adapter *adapter)
 	default:
 		/* Other devices aren't supported */
 		return;
-	};
+	}
 
 	IXGBE_WRITE_FLUSH(hw);
 }
-- 
2.35.1

[PATCH net-next 4/6] ixgbevf: Add error messages on vlan error

[Tony Nguyen]

From: Jan Sokolowski <jan.sokolowski@intel.com>

ixgbevf did not provide an error in dmesg if VLAN addition failed.

Add two descriptive failure messages in the kernel log.

Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c   | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index be733677bdc8..0aaf70c063da 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -2044,12 +2044,16 @@ static int ixgbevf_vlan_rx_add_vid(struct net_device *netdev,
 
 	spin_unlock_bh(&adapter->mbx_lock);
 
-	/* translate error return types so error makes sense */
-	if (err == IXGBE_ERR_MBX)
-		return -EIO;
+	if (err) {
+		netdev_err(netdev, "VF could not set VLAN %d\n", vid);
+
+		/* translate error return types so error makes sense */
+		if (err == IXGBE_ERR_MBX)
+			return -EIO;
 
-	if (err == IXGBE_ERR_INVALID_ARGUMENT)
-		return -EACCES;
+		if (err == IXGBE_ERR_INVALID_ARGUMENT)
+			return -EACCES;
+	}
 
 	set_bit(vid, adapter->active_vlans);
 
@@ -2070,6 +2074,9 @@ static int ixgbevf_vlan_rx_kill_vid(struct net_device *netdev,
 
 	spin_unlock_bh(&adapter->mbx_lock);
 
+	if (err)
+		netdev_err(netdev, "Could not remove VLAN %d\n", vid);
+
 	clear_bit(vid, adapter->active_vlans);
 
 	return err;
-- 
2.35.1

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

[Tony Nguyen]

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)
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;
 	} else {
 		memset(q_vector, 0, size);
 	}
-- 
2.35.1

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

[Leon Romanovsky]

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.


>  	} else {
>  		memset(q_vector, 0, size);
>  	}
> -- 
> 2.35.1
> 

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

[Ruhl, Michael J]

>-----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?

Mike

>
>>  	} else {
>>  		memset(q_vector, 0, size);
>>  	}
>> --
>> 2.35.1
>>

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

[Leon Romanovsky]

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

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

[Jacob Keller]

On 11/7/2022 9:45 AM, Leon Romanovsky wrote:
> 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

the uses of kfree_rcu were introduced by 5536d2102a2d ("igb: Combine 
q_vector and ring allocation into a single function")

The commit doesn't mention switching from kfree to kfree_rcu and I 
suspect that the igb driver is not actually really using RCU semantics 
properly.

The closest explanation is that the get_stats64 function might be 
accessing the ring and thus needs the RCU grace period.. but I think 
you're right in that we're missing the necessary RCU access macros.

Thanks,
Jake

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

[Jakub Kicinski]

On Mon, 7 Nov 2022 10:35:14 -0800 Jacob Keller wrote:
> > 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.
> 
> the uses of kfree_rcu were introduced by 5536d2102a2d ("igb: Combine 
> q_vector and ring allocation into a single function")
> 
> The commit doesn't mention switching from kfree to kfree_rcu and I 
> suspect that the igb driver is not actually really using RCU semantics 
> properly.
> 
> The closest explanation is that the get_stats64 function might be 
> accessing the ring and thus needs the RCU grace period.. but I think 
> you're right in that we're missing the necessary RCU access macros.

Alright, expecting a follow up for this.

[PATCH net-next 6/6] igb: Proactively round up to kmalloc bucket size

[Tony Nguyen]

From: Kees Cook <keescook@chromium.org>

In preparation for removing the "silently change allocation size"
users of ksize(), explicitly round up all q_vector allocations so that
allocations can be correctly compared to ksize().

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>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index c2bb658198bf..97290fc0fddd 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1195,7 +1195,7 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter,
 		return -ENOMEM;
 
 	ring_count = txr_count + rxr_count;
-	size = struct_size(q_vector, ring, ring_count);
+	size = kmalloc_size_roundup(struct_size(q_vector, ring, ring_count));
 
 	/* allocate q_vector and rings */
 	q_vector = adapter->q_vector[v_idx];
-- 
2.35.1

Re: [PATCH net-next 0/6][pull request] Intel Wired LAN Driver Updates 2022-11-04 (ixgbe, ixgbevf, igb)

[Leon Romanovsky]

On Fri, Nov 04, 2022 at 01:54:08PM -0700, Tony Nguyen wrote:
> This series contains updates to ixgbe, ixgbevf, and igb drivers.

<...>

> Kees Cook (2):
>   igb: Do not free q_vector unless new one was allocated

Except this patch and wrong Tested-by tag.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next 0/6][pull request] Intel Wired LAN Driver Updates 2022-11-04 (ixgbe, ixgbevf, igb)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Fri,  4 Nov 2022 13:54:08 -0700 you wrote:
> This series contains updates to ixgbe, ixgbevf, and igb drivers.
> 
> Daniel Willenson adjusts descriptor buffer limits to be based on what
> hardware supports instead of using a generic, least common value for
> ixgbe.
> 
> Ani removes local variable for ixgbe, instead returning conditional result
> directly.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] ixgbe: change MAX_RXD/MAX_TXD based on adapter type
    https://git.kernel.org/netdev/net-next/c/864f88884c42
  - [net-next,2/6] ixgbe: Remove local variable
    https://git.kernel.org/netdev/net-next/c/6a6f9e3e03ae
  - [net-next,3/6] ixgbe: Remove unneeded semicolon
    https://git.kernel.org/netdev/net-next/c/f49fafa504d5
  - [net-next,4/6] ixgbevf: Add error messages on vlan error
    https://git.kernel.org/netdev/net-next/c/eac0b6804b74
  - [net-next,5/6] igb: Do not free q_vector unless new one was allocated
    https://git.kernel.org/netdev/net-next/c/0668716506ca
  - [net-next,6/6] igb: Proactively round up to kmalloc bucket size
    https://git.kernel.org/netdev/net-next/c/e9d696cb44e3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html