[PATCH] um: vector: fix return value check in vector_legacy_rx

[PATCH] um: vector: fix return value check in vector_legacy_rx

[Ma Ke]

In vector_legacy_rx, to avoid an unexpected result returned by
pskb_trim, we should check the return value of pskb_trim().

Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
 arch/um/drivers/vector_kern.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 131b7cb29576..822a8c0cdcc1 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -890,7 +890,8 @@ static int vector_legacy_rx(struct vector_private *vp)
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				}
 			}
-			pskb_trim(skb, pkt_len - vp->rx_header_size);
+			if (pskb_trim(skb, pkt_len - vp->rx_header_size))
+				return 0;
 			skb->protocol = eth_type_trans(skb, skb->dev);
 			vp->dev->stats.rx_bytes += skb->len;
 			vp->dev->stats.rx_packets++;
-- 
2.37.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] um: vector: fix return value check in vector_legacy_rx

[Richard Weinberger]

On Fri, Oct 6, 2023 at 2:28 PM Ma Ke <make_ruc2021@163.com> wrote:
>
> In vector_legacy_rx, to avoid an unexpected result returned by
> pskb_trim, we should check the return value of pskb_trim().
>
> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> ---
>  arch/um/drivers/vector_kern.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> index 131b7cb29576..822a8c0cdcc1 100644
> --- a/arch/um/drivers/vector_kern.c
> +++ b/arch/um/drivers/vector_kern.c
> @@ -890,7 +890,8 @@ static int vector_legacy_rx(struct vector_private *vp)
>                                         skb->ip_summed = CHECKSUM_UNNECESSARY;
>                                 }
>                         }
> -                       pskb_trim(skb, pkt_len - vp->rx_header_size);
> +                       if (pskb_trim(skb, pkt_len - vp->rx_header_size))
> +                               return 0;

I think this adds a memory leak. Also, can pskb_trim() really fail in
this scenario?
The function controls skb creation and knows all lengths.

Re: [PATCH] um: vector: fix return value check in vector_legacy_rx

[Johannes Berg]

On Thu, 2024-01-04 at 22:05 +0100, Richard Weinberger wrote:
> On Fri, Oct 6, 2023 at 2:28 PM Ma Ke <make_ruc2021@163.com> wrote:
> > 
> > In vector_legacy_rx, to avoid an unexpected result returned by
> > pskb_trim, we should check the return value of pskb_trim().
> > 
> > Signed-off-by: Ma Ke <make_ruc2021@163.com>
> > ---
> >  arch/um/drivers/vector_kern.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> > index 131b7cb29576..822a8c0cdcc1 100644
> > --- a/arch/um/drivers/vector_kern.c
> > +++ b/arch/um/drivers/vector_kern.c
> > @@ -890,7 +890,8 @@ static int vector_legacy_rx(struct vector_private *vp)
> >                                         skb->ip_summed = CHECKSUM_UNNECESSARY;
> >                                 }
> >                         }
> > -                       pskb_trim(skb, pkt_len - vp->rx_header_size);
> > +                       if (pskb_trim(skb, pkt_len - vp->rx_header_size))
> > +                               return 0;
> 
> I think this adds a memory leak. Also, can pskb_trim() really fail in
> this scenario?
> The function controls skb creation and knows all lengths.
> 
We had pretty much the exact same discussion in the other patch ...

https://patchwork.ozlabs.org/project/linux-um/patch/20231007005104.3994678-1-make_ruc2021@163.com/


No point arguing with people who care about static checkers only, I
guess. This person here never even came back to respond to the comments,
my take is they're throwing patches over the wall they didn't think
about, just to see what sticks.

johannes

Re: [PATCH] um: vector: fix return value check in vector_legacy_rx

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Johannes Berg" <johannes@sipsolutions.net>
> An: "Richard Weinberger" <richard.weinberger@gmail.com>, "Ma Ke" <make_ruc2021@163.com>
> CC: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, xiangyang3@huawei.com, "linux-um"
> <linux-um@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>
> Gesendet: Freitag, 5. Januar 2024 09:42:12
> Betreff: Re: [PATCH] um: vector: fix return value check in vector_legacy_rx

> On Thu, 2024-01-04 at 22:05 +0100, Richard Weinberger wrote:
>> On Fri, Oct 6, 2023 at 2:28 PM Ma Ke <make_ruc2021@163.com> wrote:
>> > 
>> > In vector_legacy_rx, to avoid an unexpected result returned by
>> > pskb_trim, we should check the return value of pskb_trim().
>> > 
>> > Signed-off-by: Ma Ke <make_ruc2021@163.com>
>> > ---
>> >  arch/um/drivers/vector_kern.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>> > index 131b7cb29576..822a8c0cdcc1 100644
>> > --- a/arch/um/drivers/vector_kern.c
>> > +++ b/arch/um/drivers/vector_kern.c
>> > @@ -890,7 +890,8 @@ static int vector_legacy_rx(struct vector_private *vp)
>> >                                         skb->ip_summed = CHECKSUM_UNNECESSARY;
>> >                                 }
>> >                         }
>> > -                       pskb_trim(skb, pkt_len - vp->rx_header_size);
>> > +                       if (pskb_trim(skb, pkt_len - vp->rx_header_size))
>> > +                               return 0;
>> 
>> I think this adds a memory leak. Also, can pskb_trim() really fail in
>> this scenario?
>> The function controls skb creation and knows all lengths.
>> 
> We had pretty much the exact same discussion in the other patch ...
> 
> https://patchwork.ozlabs.org/project/linux-um/patch/20231007005104.3994678-1-make_ruc2021@163.com/

Yeah, I saw that discussion after commenting to this patch.
(My fault, I scanned patchwork bottom up) 

> 
> No point arguing with people who care about static checkers only, I
> guess. This person here never even came back to respond to the comments,
> my take is they're throwing patches over the wall they didn't think
> about, just to see what sticks.

Drive-by fixes are often a waste of everyone's time. :-(

Thanks,
//richard

Re: [PATCH] um: vector: fix return value check in vector_legacy_rx

[Anton Ivanov]

On 05/01/2024 08:42, Johannes Berg wrote:
> On Thu, 2024-01-04 at 22:05 +0100, Richard Weinberger wrote:
>> On Fri, Oct 6, 2023 at 2:28 PM Ma Ke <make_ruc2021@163.com> wrote:
>>>
>>> In vector_legacy_rx, to avoid an unexpected result returned by
>>> pskb_trim, we should check the return value of pskb_trim().
>>>
>>> Signed-off-by: Ma Ke <make_ruc2021@163.com>
>>> ---
>>>   arch/um/drivers/vector_kern.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>>> index 131b7cb29576..822a8c0cdcc1 100644
>>> --- a/arch/um/drivers/vector_kern.c
>>> +++ b/arch/um/drivers/vector_kern.c
>>> @@ -890,7 +890,8 @@ static int vector_legacy_rx(struct vector_private *vp)
>>>                                          skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>                                  }
>>>                          }
>>> -                       pskb_trim(skb, pkt_len - vp->rx_header_size);
>>> +                       if (pskb_trim(skb, pkt_len - vp->rx_header_size))
>>> +                               return 0;
>>
>> I think this adds a memory leak. Also, can pskb_trim() really fail in
>> this scenario?
>> The function controls skb creation and knows all lengths.
>>
> We had pretty much the exact same discussion in the other patch ...
> 
> https://patchwork.ozlabs.org/project/linux-um/patch/20231007005104.3994678-1-make_ruc2021@163.com/
> 
> 
> No point arguing with people who care about static checkers only, I
> guess. This person here never even came back to respond to the comments,
> my take is they're throwing patches over the wall they didn't think
> about, just to see what sticks.

CV padding, the 2024 way. Accomplishments: submitted 231 patches to the 
linux kernel.

> 
> johannes
> 
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/