[PATCH v1] gve: fix zero size queue page list allocation

[PATCH v1] gve: fix zero size queue page list allocation

[Haiyue Wang]

According to the two functions 'gve_num_tx/rx_qpls', only the queue with
GVE_GQI_QPL_FORMAT format has queue page list.

The 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero sized
memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.

The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
address, so the driver can run successfully. Also the code still checks
the queue page list number firstly, then accesses the allocated memory,
so zero number queue page list allocation will not lead to access fault.

Use the queue page list number to detect no QPLs, it can avoid zero size
queue page list memory allocation.

Fixes: a5886ef4f4bf ("gve: Introduce per netdev `enum gve_queue_format`")
Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 54e51c8221b8..6cafee55efc3 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -857,8 +857,7 @@ static int gve_alloc_qpls(struct gve_priv *priv)
 	int i, j;
 	int err;
 
-	/* Raw addressing means no QPLs */
-	if (priv->queue_format == GVE_GQI_RDA_FORMAT)
+	if (num_qpls == 0)
 		return 0;
 
 	priv->qpls = kvcalloc(num_qpls, sizeof(*priv->qpls), GFP_KERNEL);
@@ -901,8 +900,7 @@ static void gve_free_qpls(struct gve_priv *priv)
 	int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
 	int i;
 
-	/* Raw addressing means no QPLs */
-	if (priv->queue_format == GVE_GQI_RDA_FORMAT)
+	if (num_qpls == 0)
 		return;
 
 	kvfree(priv->qpl_cfg.qpl_id_map);
-- 
2.35.1

[PATCH net-next v2] gve: enhance no queue page list detection

[Haiyue Wang]

The commit
a5886ef4f4bf ("gve: Introduce per netdev `enum gve_queue_format`")
introduces three queue format type, only GVE_GQI_QPL_FORMAT queue has
page list. So it should use the queue page list number to detect the
zero size queue page list. Correct the design logic.

Using the 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero
sized memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.

The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
address, so the driver can run successfully. Also the code still checks
the queue page list number firstly, then accesses the allocated memory,
so zero number queue page list allocation will not lead to access fault.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 54e51c8221b8..6cafee55efc3 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -857,8 +857,7 @@ static int gve_alloc_qpls(struct gve_priv *priv)
 	int i, j;
 	int err;
 
-	/* Raw addressing means no QPLs */
-	if (priv->queue_format == GVE_GQI_RDA_FORMAT)
+	if (num_qpls == 0)
 		return 0;
 
 	priv->qpls = kvcalloc(num_qpls, sizeof(*priv->qpls), GFP_KERNEL);
@@ -901,8 +900,7 @@ static void gve_free_qpls(struct gve_priv *priv)
 	int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
 	int i;
 
-	/* Raw addressing means no QPLs */
-	if (priv->queue_format == GVE_GQI_RDA_FORMAT)
+	if (num_qpls == 0)
 		return;
 
 	kvfree(priv->qpl_cfg.qpl_id_map);
-- 
2.35.1

Re: [PATCH v1] gve: fix zero size queue page list allocation

[Jakub Kicinski]

On Mon, 14 Feb 2022 10:41:29 +0800 Haiyue Wang wrote:
> According to the two functions 'gve_num_tx/rx_qpls', only the queue with
> GVE_GQI_QPL_FORMAT format has queue page list.
> 
> The 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero sized
> memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.
> 
> The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
> address, so the driver can run successfully. Also the code still checks
> the queue page list number firstly, then accesses the allocated memory,
> so zero number queue page list allocation will not lead to access fault.
> 
> Use the queue page list number to detect no QPLs, it can avoid zero size
> queue page list memory allocation.

There's no bug here, strictly speaking, the driver will function
correctly? In that case please repost without the Fixes tag and
with [PATCH net-next] in the subject.

> Fixes: a5886ef4f4bf ("gve: Introduce per netdev `enum gve_queue_format`")
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>

RE: [PATCH v1] gve: fix zero size queue page list allocation

[Wang, Haiyue]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, February 15, 2022 13:22
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: netdev@vger.kernel.org; Jeroen de Borst <jeroendb@google.com>; Catherine Sullivan
> <csully@google.com>; David Awogbemila <awogbemila@google.com>; David S. Miller <davem@davemloft.net>;
> Willem de Bruijn <willemb@google.com>; Forrest, Bailey <bcf@google.com>; Tao Liu <xliutaox@google.com>;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>; John Fraker <jfraker@google.com>; Yangchun Fu
> <yangchun@google.com>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v1] gve: fix zero size queue page list allocation
> 
> On Mon, 14 Feb 2022 10:41:29 +0800 Haiyue Wang wrote:
> > According to the two functions 'gve_num_tx/rx_qpls', only the queue with
> > GVE_GQI_QPL_FORMAT format has queue page list.
> >
> > The 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero sized
> > memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.
> >
> > The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
> > address, so the driver can run successfully. Also the code still checks
> > the queue page list number firstly, then accesses the allocated memory,
> > so zero number queue page list allocation will not lead to access fault.
> >
> > Use the queue page list number to detect no QPLs, it can avoid zero size
> > queue page list memory allocation.
> 
> There's no bug here, strictly speaking, the driver will function
> correctly? In that case please repost without the Fixes tag and

Code design bug, the 'queue_format == GVE_GQI_RDA_FORMAT' is not correct. But,
yes, it works. So still need to remove the tag ?

> with [PATCH net-next] in the subject.
> 
> > Fixes: a5886ef4f4bf ("gve: Introduce per netdev `enum gve_queue_format`")
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>

Re: [PATCH v1] gve: fix zero size queue page list allocation

[Jakub Kicinski]

On Tue, 15 Feb 2022 05:25:49 +0000 Wang, Haiyue wrote:
> > On Mon, 14 Feb 2022 10:41:29 +0800 Haiyue Wang wrote:  
> > > According to the two functions 'gve_num_tx/rx_qpls', only the queue with
> > > GVE_GQI_QPL_FORMAT format has queue page list.
> > >
> > > The 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero sized
> > > memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.
> > >
> > > The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
> > > address, so the driver can run successfully. Also the code still checks
> > > the queue page list number firstly, then accesses the allocated memory,
> > > so zero number queue page list allocation will not lead to access fault.
> > >
> > > Use the queue page list number to detect no QPLs, it can avoid zero size
> > > queue page list memory allocation.  
> > 
> > There's no bug here, strictly speaking, the driver will function
> > correctly? In that case please repost without the Fixes tag and  
> 
> Code design bug, the 'queue_format == GVE_GQI_RDA_FORMAT' is not correct. But,
> yes, it works. So still need to remove the tag ?

A bug is something that users can notice. If there are conditions under
which this may lead to user-visible mis-behavior then we should keep
the tag and send the patch to stable as well.

If there is no user-visible problem here, then the patch is just
future-proofing / refactoring and we should not risk introducing real
bugs by making people backport it (which is what adding a Fixes will
do).

RE: [PATCH v1] gve: fix zero size queue page list allocation

[Wang, Haiyue]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, February 15, 2022 13:31
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: netdev@vger.kernel.org; Jeroen de Borst <jeroendb@google.com>; Catherine Sullivan
> <csully@google.com>; David Awogbemila <awogbemila@google.com>; David S. Miller <davem@davemloft.net>;
> Willem de Bruijn <willemb@google.com>; Forrest, Bailey <bcf@google.com>; Tao Liu <xliutaox@google.com>;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>; John Fraker <jfraker@google.com>; Yangchun Fu
> <yangchun@google.com>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v1] gve: fix zero size queue page list allocation
> 
> On Tue, 15 Feb 2022 05:25:49 +0000 Wang, Haiyue wrote:
> > > On Mon, 14 Feb 2022 10:41:29 +0800 Haiyue Wang wrote:
> > > > According to the two functions 'gve_num_tx/rx_qpls', only the queue with
> > > > GVE_GQI_QPL_FORMAT format has queue page list.
> > > >
> > > > The 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero sized
> > > > memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.
> > > >
> > > > The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
> > > > address, so the driver can run successfully. Also the code still checks
> > > > the queue page list number firstly, then accesses the allocated memory,
> > > > so zero number queue page list allocation will not lead to access fault.
> > > >
> > > > Use the queue page list number to detect no QPLs, it can avoid zero size
> > > > queue page list memory allocation.
> > >
> > > There's no bug here, strictly speaking, the driver will function
> > > correctly? In that case please repost without the Fixes tag and
> >
> > Code design bug, the 'queue_format == GVE_GQI_RDA_FORMAT' is not correct. But,
> > yes, it works. So still need to remove the tag ?
> 
> A bug is something that users can notice. If there are conditions under
> which this may lead to user-visible mis-behavior then we should keep
> the tag and send the patch to stable as well.
> 
> If there is no user-visible problem here, then the patch is just
> future-proofing / refactoring and we should not risk introducing real
> bugs by making people backport it (which is what adding a Fixes will
> do).

OK. Will remove the tag in v2.

Re: [PATCH net-next v2] gve: enhance no queue page list detection

[Bailey Forrest]

On Mon, Feb 14, 2022 at 9:52 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
>
> The commit
> a5886ef4f4bf ("gve: Introduce per netdev `enum gve_queue_format`")
> introduces three queue format type, only GVE_GQI_QPL_FORMAT queue has
> page list. So it should use the queue page list number to detect the
> zero size queue page list. Correct the design logic.
>
> Using the 'queue_format == GVE_GQI_RDA_FORMAT' may lead to request zero
> sized memory allocation, like if the queue format is GVE_DQO_RDA_FORMAT.
>
> The kernel memory subsystem will return ZERO_SIZE_PTR, which is not NULL
> address, so the driver can run successfully. Also the code still checks
> the queue page list number firstly, then accesses the allocated memory,
> so zero number queue page list allocation will not lead to access fault.
>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>

Reviewed-by: Bailey Forrest <bcf@google.com>