* [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
@ 2022-11-17 7:50 Zhang Changzhong
2022-11-17 11:36 ` Leon Romanovsky
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Zhang Changzhong @ 2022-11-17 7:50 UTC (permalink / raw)
To: Edward Cree, Martin Habets, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Zhang Changzhong, netdev, linux-kernel
The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
in error handling case, add dev_kfree_skb_any() to fix it.
Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
index 88fa295..ddcc325 100644
--- a/drivers/net/ethernet/sfc/ef100_netdev.c
+++ b/drivers/net/ethernet/sfc/ef100_netdev.c
@@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
skb->len, skb->data_len, channel->channel);
if (!efx->n_channels || !efx->n_tx_channels || !channel) {
netif_stop_queue(net_dev);
+ dev_kfree_skb_any(skb);
goto err;
}
--
2.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 7:50 [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit() Zhang Changzhong
@ 2022-11-17 11:36 ` Leon Romanovsky
2022-11-17 12:41 ` Zhang Changzhong
2022-11-18 9:13 ` Martin Habets
2022-11-22 10:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2022-11-17 11:36 UTC (permalink / raw)
To: Zhang Changzhong
Cc: Edward Cree, Martin Habets, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> in error handling case, add dev_kfree_skb_any() to fix it.
>
> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> ---
> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> index 88fa295..ddcc325 100644
> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
> skb->len, skb->data_len, channel->channel);
> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
> netif_stop_queue(net_dev);
> + dev_kfree_skb_any(skb);
> goto err;
> }
ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
NULL or ERR at this stage.
diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
index 29ffaf35559d..426706b91d02 100644
--- a/drivers/net/ethernet/sfc/ef100_tx.c
+++ b/drivers/net/ethernet/sfc/ef100_tx.c
@@ -497,7 +497,7 @@ int __ef100_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
err:
efx_enqueue_unwind(tx_queue, old_insert_count);
- if (!IS_ERR_OR_NULL(skb))
+ if (rc)
dev_kfree_skb_any(skb);
/* If we're not expecting another transmit and we had something to push
>
> --
> 2.9.5
>
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 11:36 ` Leon Romanovsky
@ 2022-11-17 12:41 ` Zhang Changzhong
2022-11-17 13:05 ` Leon Romanovsky
0 siblings, 1 reply; 11+ messages in thread
From: Zhang Changzhong @ 2022-11-17 12:41 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Edward Cree, Martin Habets, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On 2022/11/17 19:36, Leon Romanovsky wrote:
> On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
>> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
>> in error handling case, add dev_kfree_skb_any() to fix it.
>>
>> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
>> ---
>> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>> index 88fa295..ddcc325 100644
>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
>> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
>> skb->len, skb->data_len, channel->channel);
>> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
>> netif_stop_queue(net_dev);
>> + dev_kfree_skb_any(skb);
>> goto err;
>> }
>
> ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
> NULL or ERR at this stage.
SKB shouldn't be NULL or ERR, so it can be freed. But this code looks weird.
>
> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
> index 29ffaf35559d..426706b91d02 100644
> --- a/drivers/net/ethernet/sfc/ef100_tx.c
> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
> @@ -497,7 +497,7 @@ int __ef100_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
>
> err:
> efx_enqueue_unwind(tx_queue, old_insert_count);
> - if (!IS_ERR_OR_NULL(skb))
> + if (rc)
> dev_kfree_skb_any(skb);
>
> /* If we're not expecting another transmit and we had something to push
>
>
>>
>> --
>> 2.9.5
>>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 12:41 ` Zhang Changzhong
@ 2022-11-17 13:05 ` Leon Romanovsky
2022-11-18 9:15 ` Martin Habets
0 siblings, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2022-11-17 13:05 UTC (permalink / raw)
To: Zhang Changzhong
Cc: Edward Cree, Martin Habets, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On Thu, Nov 17, 2022 at 08:41:52PM +0800, Zhang Changzhong wrote:
>
>
> On 2022/11/17 19:36, Leon Romanovsky wrote:
> > On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
> >> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> >> in error handling case, add dev_kfree_skb_any() to fix it.
> >>
> >> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> >> ---
> >> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> >> index 88fa295..ddcc325 100644
> >> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> >> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> >> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
> >> skb->len, skb->data_len, channel->channel);
> >> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
> >> netif_stop_queue(net_dev);
> >> + dev_kfree_skb_any(skb);
> >> goto err;
> >> }
> >
> > ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
> > NULL or ERR at this stage.
>
> SKB shouldn't be NULL or ERR, so it can be freed. But this code looks weird.
Please take a look __ef100_enqueue_skb() and see if it frees SKB on
error or not. If not, please fix it.
Thanks
>
> >
> > diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
> > index 29ffaf35559d..426706b91d02 100644
> > --- a/drivers/net/ethernet/sfc/ef100_tx.c
> > +++ b/drivers/net/ethernet/sfc/ef100_tx.c
> > @@ -497,7 +497,7 @@ int __ef100_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
> >
> > err:
> > efx_enqueue_unwind(tx_queue, old_insert_count);
> > - if (!IS_ERR_OR_NULL(skb))
> > + if (rc)
> > dev_kfree_skb_any(skb);
> >
> > /* If we're not expecting another transmit and we had something to push
> >
> >
> >>
> >> --
> >> 2.9.5
> >>
> > .
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 7:50 [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit() Zhang Changzhong
2022-11-17 11:36 ` Leon Romanovsky
@ 2022-11-18 9:13 ` Martin Habets
2022-11-22 10:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 11+ messages in thread
From: Martin Habets @ 2022-11-18 9:13 UTC (permalink / raw)
To: Zhang Changzhong
Cc: Edward Cree, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel
On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> in error handling case, add dev_kfree_skb_any() to fix it.
>
> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
> ---
> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> index 88fa295..ddcc325 100644
> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
> skb->len, skb->data_len, channel->channel);
> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
> netif_stop_queue(net_dev);
> + dev_kfree_skb_any(skb);
> goto err;
> }
>
> --
> 2.9.5
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 13:05 ` Leon Romanovsky
@ 2022-11-18 9:15 ` Martin Habets
2022-11-18 11:53 ` Zhang Changzhong
2022-11-18 17:11 ` Leon Romanovsky
0 siblings, 2 replies; 11+ messages in thread
From: Martin Habets @ 2022-11-18 9:15 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Zhang Changzhong, Edward Cree, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On Thu, Nov 17, 2022 at 03:05:27PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 17, 2022 at 08:41:52PM +0800, Zhang Changzhong wrote:
> >
> >
> > On 2022/11/17 19:36, Leon Romanovsky wrote:
> > > On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
> > >> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> > >> in error handling case, add dev_kfree_skb_any() to fix it.
> > >>
> > >> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> > >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> > >> ---
> > >> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> > >> index 88fa295..ddcc325 100644
> > >> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> > >> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> > >> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
> > >> skb->len, skb->data_len, channel->channel);
> > >> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
> > >> netif_stop_queue(net_dev);
> > >> + dev_kfree_skb_any(skb);
> > >> goto err;
> > >> }
> > >
> > > ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
> > > NULL or ERR at this stage.
> >
> > SKB shouldn't be NULL or ERR, so it can be freed. But this code looks weird.
>
> Please take a look __ef100_enqueue_skb() and see if it frees SKB on
> error or not. If not, please fix it.
That function looks ok to me, but I appreciate the extra eyes on it.
Martin
> Thanks
>
> >
> > >
> > > diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
> > > index 29ffaf35559d..426706b91d02 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_tx.c
> > > +++ b/drivers/net/ethernet/sfc/ef100_tx.c
> > > @@ -497,7 +497,7 @@ int __ef100_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
> > >
> > > err:
> > > efx_enqueue_unwind(tx_queue, old_insert_count);
> > > - if (!IS_ERR_OR_NULL(skb))
> > > + if (rc)
> > > dev_kfree_skb_any(skb);
> > >
> > > /* If we're not expecting another transmit and we had something to push
> > >
> > >
> > >>
> > >> --
> > >> 2.9.5
> > >>
> > > .
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-18 9:15 ` Martin Habets
@ 2022-11-18 11:53 ` Zhang Changzhong
2022-11-18 17:11 ` Leon Romanovsky
1 sibling, 0 replies; 11+ messages in thread
From: Zhang Changzhong @ 2022-11-18 11:53 UTC (permalink / raw)
To: Leon Romanovsky, Edward Cree, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On 2022/11/18 17:15, Martin Habets wrote:
> On Thu, Nov 17, 2022 at 03:05:27PM +0200, Leon Romanovsky wrote:
>> On Thu, Nov 17, 2022 at 08:41:52PM +0800, Zhang Changzhong wrote:
>>>
>>>
>>> On 2022/11/17 19:36, Leon Romanovsky wrote:
>>>> On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
>>>>> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
>>>>> in error handling case, add dev_kfree_skb_any() to fix it.
>>>>>
>>>>> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
>>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
>>>>> ---
>>>>> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>>>>> index 88fa295..ddcc325 100644
>>>>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>>>>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
>>>>> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
>>>>> skb->len, skb->data_len, channel->channel);
>>>>> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
>>>>> netif_stop_queue(net_dev);
>>>>> + dev_kfree_skb_any(skb);
>>>>> goto err;
>>>>> }
>>>>
>>>> ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
>>>> NULL or ERR at this stage.
>>>
>>> SKB shouldn't be NULL or ERR, so it can be freed. But this code looks weird.
>>
>> Please take a look __ef100_enqueue_skb() and see if it frees SKB on
>> error or not. If not, please fix it.
>
> That function looks ok to me, but I appreciate the extra eyes on it.
>
I've looked at this function and it seems every error handling path frees the skb.
Thanks,
Changzhong
> Martin
>
>> Thanks
>>
>>>
>>>>
>>>> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
>>>> index 29ffaf35559d..426706b91d02 100644
>>>> --- a/drivers/net/ethernet/sfc/ef100_tx.c
>>>> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
>>>> @@ -497,7 +497,7 @@ int __ef100_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
>>>>
>>>> err:
>>>> efx_enqueue_unwind(tx_queue, old_insert_count);
>>>> - if (!IS_ERR_OR_NULL(skb))
>>>> + if (rc)
>>>> dev_kfree_skb_any(skb);
>>>>
>>>> /* If we're not expecting another transmit and we had something to push
>>>>
>>>>
>>>>>
>>>>> --
>>>>> 2.9.5
>>>>>
>>>> .
>>>>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-18 9:15 ` Martin Habets
2022-11-18 11:53 ` Zhang Changzhong
@ 2022-11-18 17:11 ` Leon Romanovsky
2022-11-22 8:28 ` Paolo Abeni
1 sibling, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2022-11-18 17:11 UTC (permalink / raw)
To: Zhang Changzhong, Edward Cree, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
On Fri, Nov 18, 2022 at 09:15:43AM +0000, Martin Habets wrote:
> On Thu, Nov 17, 2022 at 03:05:27PM +0200, Leon Romanovsky wrote:
> > On Thu, Nov 17, 2022 at 08:41:52PM +0800, Zhang Changzhong wrote:
> > >
> > >
> > > On 2022/11/17 19:36, Leon Romanovsky wrote:
> > > > On Thu, Nov 17, 2022 at 03:50:09PM +0800, Zhang Changzhong wrote:
> > > >> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> > > >> in error handling case, add dev_kfree_skb_any() to fix it.
> > > >>
> > > >> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> > > >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> > > >> ---
> > > >> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> > > >> 1 file changed, 1 insertion(+)
> > > >>
> > > >> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> > > >> index 88fa295..ddcc325 100644
> > > >> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> > > >> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> > > >> @@ -218,6 +218,7 @@ netdev_tx_t __ef100_hard_start_xmit(struct sk_buff *skb,
> > > >> skb->len, skb->data_len, channel->channel);
> > > >> if (!efx->n_channels || !efx->n_tx_channels || !channel) {
> > > >> netif_stop_queue(net_dev);
> > > >> + dev_kfree_skb_any(skb);
> > > >> goto err;
> > > >> }
> > > >
> > > > ef100 doesn't release in __ef100_enqueue_skb() either. SKB shouldn't be
> > > > NULL or ERR at this stage.
> > >
> > > SKB shouldn't be NULL or ERR, so it can be freed. But this code looks weird.
> >
> > Please take a look __ef100_enqueue_skb() and see if it frees SKB on
> > error or not. If not, please fix it.
>
> That function looks ok to me, but I appreciate the extra eyes on it.
__ef100_enqueue_skb() has the following check in error path:
498 err:
499 efx_enqueue_unwind(tx_queue, old_insert_count);
500 if (!IS_ERR_OR_NULL(skb))
501 dev_kfree_skb_any(skb);
502
The issue is that skb is never error or null here and this "if" is
actually always true and can be deleted.
Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-18 17:11 ` Leon Romanovsky
@ 2022-11-22 8:28 ` Paolo Abeni
2022-11-22 9:47 ` Leon Romanovsky
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2022-11-22 8:28 UTC (permalink / raw)
To: Leon Romanovsky, Zhang Changzhong, Edward Cree, David S. Miller,
Eric Dumazet, Jakub Kicinski, netdev, linux-kernel
Hello,
On Fri, 2022-11-18 at 19:11 +0200, Leon Romanovsky wrote:
> On Fri, Nov 18, 2022 at 09:15:43AM +0000, Martin Habets wrote:
> > On Thu, Nov 17, 2022 at 03:05:27PM +0200, Leon Romanovsky wrote:
> > > Please take a look __ef100_enqueue_skb() and see if it frees SKB on
> > > error or not. If not, please fix it.
> >
> > That function looks ok to me, but I appreciate the extra eyes on it.
>
> __ef100_enqueue_skb() has the following check in error path:
>
> 498 err:
> 499 efx_enqueue_unwind(tx_queue, old_insert_count);
> 500 if (!IS_ERR_OR_NULL(skb))
> 501 dev_kfree_skb_any(skb);
> 502
>
> The issue is that skb is never error or null here and this "if" is
> actually always true and can be deleted.
I think that such additional change could be suite for a different net-
next patch, while this -net patch could land as is, @Leon: do you
agree?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-22 8:28 ` Paolo Abeni
@ 2022-11-22 9:47 ` Leon Romanovsky
0 siblings, 0 replies; 11+ messages in thread
From: Leon Romanovsky @ 2022-11-22 9:47 UTC (permalink / raw)
To: Paolo Abeni
Cc: Zhang Changzhong, Edward Cree, David S. Miller, Eric Dumazet,
Jakub Kicinski, netdev, linux-kernel
On Tue, Nov 22, 2022 at 09:28:42AM +0100, Paolo Abeni wrote:
> Hello,
>
> On Fri, 2022-11-18 at 19:11 +0200, Leon Romanovsky wrote:
> > On Fri, Nov 18, 2022 at 09:15:43AM +0000, Martin Habets wrote:
> > > On Thu, Nov 17, 2022 at 03:05:27PM +0200, Leon Romanovsky wrote:
> > > > Please take a look __ef100_enqueue_skb() and see if it frees SKB on
> > > > error or not. If not, please fix it.
> > >
> > > That function looks ok to me, but I appreciate the extra eyes on it.
> >
> > __ef100_enqueue_skb() has the following check in error path:
> >
> > 498 err:
> > 499 efx_enqueue_unwind(tx_queue, old_insert_count);
> > 500 if (!IS_ERR_OR_NULL(skb))
> > 501 dev_kfree_skb_any(skb);
> > 502
> >
> > The issue is that skb is never error or null here and this "if" is
> > actually always true and can be deleted.
>
> I think that such additional change could be suite for a different net-
> next patch, while this -net patch could land as is, @Leon: do you
> agree?
>
Sure, thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit()
2022-11-17 7:50 [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit() Zhang Changzhong
2022-11-17 11:36 ` Leon Romanovsky
2022-11-18 9:13 ` Martin Habets
@ 2022-11-22 10:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-11-22 10:40 UTC (permalink / raw)
To: Zhang Changzhong
Cc: ecree.xilinx, habetsm.xilinx, davem, edumazet, kuba, pabeni,
netdev, linux-kernel
Hello:
This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:
On Thu, 17 Nov 2022 15:50:09 +0800 you wrote:
> The __ef100_hard_start_xmit() returns NETDEV_TX_OK without freeing skb
> in error handling case, add dev_kfree_skb_any() to fix it.
>
> Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> ---
> drivers/net/ethernet/sfc/ef100_netdev.c | 1 +
> 1 file changed, 1 insertion(+)
Here is the summary with links:
- [net] sfc: fix potential memleak in __ef100_hard_start_xmit()
https://git.kernel.org/netdev/net/c/aad98abd5cb8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-22 10:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-17 7:50 [PATCH net] sfc: fix potential memleak in __ef100_hard_start_xmit() Zhang Changzhong
2022-11-17 11:36 ` Leon Romanovsky
2022-11-17 12:41 ` Zhang Changzhong
2022-11-17 13:05 ` Leon Romanovsky
2022-11-18 9:15 ` Martin Habets
2022-11-18 11:53 ` Zhang Changzhong
2022-11-18 17:11 ` Leon Romanovsky
2022-11-22 8:28 ` Paolo Abeni
2022-11-22 9:47 ` Leon Romanovsky
2022-11-18 9:13 ` Martin Habets
2022-11-22 10:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).