gve_alloc_pending_packet() can return NULL, but gve_tx_add_skb_dqo()
did not check for this case before dereferencing the returned pointer.

Add a missing NULL check to prevent a potential NULL pointer
dereference when allocation fails.

This improves robustness in low-memory scenarios.

Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index a27f1574a733..9d705d94b065 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -764,6 +764,9 @@ static int gve_tx_add_skb_dqo(struct gve_tx_ring *tx,
 	s16 completion_tag;
 
 	pkt = gve_alloc_pending_packet(tx);
+	if (!pkt)
+		return -ENOMEM;
+
 	pkt->skb = skb;
 	completion_tag = pkt - tx->dqo.pending_packets;
 
-- 
2.47.1

On Sun, Jun 1, 2025 at 12:34 PM Alok Tiwari <alok.a.tiwari@oracle.com> wrote:
>
> gve_alloc_pending_packet() can return NULL, but gve_tx_add_skb_dqo()
> did not check for this case before dereferencing the returned pointer.
>
> Add a missing NULL check to prevent a potential NULL pointer
> dereference when allocation fails.
>
> This improves robustness in low-memory scenarios.
>
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>

Patch itself looks good to me, but if you can, please designate it to
the net tree by prefixing the patch with `[PATCH net v2]` as mentioned
in our docs:

https://docs.kernel.org/process/maintainer-netdev.html

Also, if possible, add `Fixes: commit a57e5de476be ("gve: DQO: Add TX
path")` to give it a chance to get picked up by stable trees.

With that:

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

On 02-06-2025 01:19, Mina Almasry wrote:
> On Sun, Jun 1, 2025 at 12:34 PM Alok Tiwari <alok.a.tiwari@oracle.com> wrote:
>>
>> gve_alloc_pending_packet() can return NULL, but gve_tx_add_skb_dqo()
>> did not check for this case before dereferencing the returned pointer.
>>
>> Add a missing NULL check to prevent a potential NULL pointer
>> dereference when allocation fails.
>>
>> This improves robustness in low-memory scenarios.
>>
>> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> 
> Patch itself looks good to me, but if you can, please designate it to
> the net tree by prefixing the patch with `[PATCH net v2]` as mentioned
> in our docs:
> 
> https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-netdev.html__;!!ACWV5N9M2RV99hQ!JPpnRT9itx84rhzAaeGelVD-bnJR8vFksx2OjGzAKZWf_A6o8hEY0CUMMUO_NuSStcCyBGnvhoJAJlADszR4D_aj$
> 
> Also, if possible, add `Fixes: commit a57e5de476be ("gve: DQO: Add TX
> path")` to give it a chance to get picked up by stable trees.
> 
> With that:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> 


Thanks for your review.

I will send v2 and add Fixes tag.
Thanks,
Alok

Hi Mina,

On 02-06-2025 01:21, ALOK TIWARI wrote:
> Patch itself looks good to me, but if you can, please designate it to
> the net tree by prefixing the patch with `[PATCH net v2]` as mentioned
> in our docs:
> 
> https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer- 
> netdev.html__;!!ACWV5N9M2RV99hQ!JPpnRT9itx84rhzAaeGelVD- 
> bnJR8vFksx2OjGzAKZWf_A6o8hEY0CUMMUO_NuSStcCyBGnvhoJAJlADszR4D_aj$
> 
> Also, if possible, add `Fixes: commit a57e5de476be ("gve: DQO: Add TX
> path")` to give it a chance to get picked up by stable trees.

I believe commit a6fb8d5a8b69 is a more natural and appropriate 
candidate for the Fixes tag compared to a57e5de476be
What’s your take on this, Mina?

> 
> With that:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>


Thanks,
Alok

On Mon, Jun 2, 2025 at 1:50 AM ALOK TIWARI <alok.a.tiwari@oracle.com> wrote:
>
> Hi Mina,
>
> On 02-06-2025 01:21, ALOK TIWARI wrote:
> > Patch itself looks good to me, but if you can, please designate it to
> > the net tree by prefixing the patch with `[PATCH net v2]` as mentioned
> > in our docs:
> >
> > https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-
> > netdev.html__;!!ACWV5N9M2RV99hQ!JPpnRT9itx84rhzAaeGelVD-
> > bnJR8vFksx2OjGzAKZWf_A6o8hEY0CUMMUO_NuSStcCyBGnvhoJAJlADszR4D_aj$
> >
> > Also, if possible, add `Fixes: commit a57e5de476be ("gve: DQO: Add TX
> > path")` to give it a chance to get picked up by stable trees.
>
> I believe commit a6fb8d5a8b69 is a more natural and appropriate
> candidate for the Fixes tag compared to a57e5de476be
> What’s your take on this, Mina?

Mina is right. Bug was added back in 2021.

Fixes: a57e5de476be ("gve: DQO: Add TX path")

Hi Alok,

I think this patch isn't needed. gve_tx_add_skb_dqo() is only called
after checking gve_maybe_stop_tx_dqo(), which checks that
gve_alloc_pending_packet() will not return NULL.

Hi Bailey,

On 03-06-2025 00:54, Bailey Forrest wrote:
> Hi Alok,
> 
> I think this patch isn't needed. gve_tx_add_skb_dqo() is only called
> after checking gve_maybe_stop_tx_dqo(), which checks that
> gve_alloc_pending_packet() will not return NULL.


Thank you for the clarification,

Even so, I felt it could be a bit misleading for developers and tools. 
But if you believe the patch isn't required,I completely understand.
In that case, I kindly request you to provide your NACK on the [PATCH 
net v2] mail thread for formal tracking,
so that other developers can also be aware of the reasoning and 
understand the context.

Appreciate your time and feedback!


Thanks,
Alok

On 6/3/25 11:03 AM, ALOK TIWARI wrote:
> On 03-06-2025 00:54, Bailey Forrest wrote:
>> I think this patch isn't needed. gve_tx_add_skb_dqo() is only called
>> after checking gve_maybe_stop_tx_dqo(), which checks that
>> gve_alloc_pending_packet() will not return NULL.
> 
> Thank you for the clarification,
> 
> Even so, I felt it could be a bit misleading for developers and tools. 
> But if you believe the patch isn't required,I completely understand.
> In that case, I kindly request you to provide your NACK on the [PATCH 
> net v2] mail thread for formal tracking,
> so that other developers can also be aware of the reasoning and 
> understand the context.

IMHO it's indeed confusing that the same condition is checked in
gve_alloc_pending_packet() and ignored by gve_tx_add_skb_dqo().

Even gve_alloc_pending_packet() is only called after the
gve_maybe_stop_tx_dqo().

Either always ignore the NULL condition it in both places (possibly with
a comment) or always check it.

/P

On Tue, Jun 3, 2025 at 3:50 AM Paolo Abeni <pabeni@redhat.com> wrote:
> IMHO it's indeed confusing that the same condition is checked in
> gve_alloc_pending_packet() and ignored by gve_tx_add_skb_dqo().
>
> Even gve_alloc_pending_packet() is only called after the
> gve_maybe_stop_tx_dqo().
>
> Either always ignore the NULL condition it in both places (possibly with
> a comment) or always check it.

It's probably not harmful to go ahead with this patch, I agree this
will make it easier for readers.

My point was it's not technically a bug, so it doesn't need (Fixes ...)

If we do make this change we can now remove the comment on gve_tx_add_skb_dqo()

 * Before this function is called, the caller must ensure
 * gve_has_pending_packet(tx) returns true.
 */

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