On 2018/6/19 14:42, Kalluru, Sudarsana wrote:
>
>
> -----Original Message-----
> From: Yunsheng Lin [mailto:linyunsh...@huawei.com]
> Sent: 19 June 2018 11:32
> To: Kalluru, Sudarsana <sudarsana.kall...@cavium.com>; da...@davemloft.net
> Cc: netdev@vger.kernel.org; Elior, Ariel <ariel.el...@cavium.com>; Kalderon,
> Michal <michal.kalde...@cavium.com>
> Subject: Re: [PATCH net 1/3] qed: Fix possible memory leak in Rx error path
> handling.
>
> External Email
>
> On 2018/6/19 12:58, Sudarsana Reddy Kalluru wrote:
>> Memory for packet buffers need to be freed in the error paths as there
>> is no consumer (e.g., upper layer) for such packets and that memory
>> will never get freed.
>> The issue was uncovered when port was attacked with flood of isatap
>> packets, these are multicast packets hence were directed at all the PFs.
>> For foce PF, this meant they were routed to the ll2 module which in
>> turn drops such packets.
>>
>> Fixes: 0a7fb11c ("qed: Add Light L2 support")
>> Signed-off-by: Sudarsana Reddy Kalluru <sudarsana.kall...@cavium.com>
>> Signed-off-by: Ariel Elior <ariel.el...@cavium.com>
>> Signed-off-by: Michal Kalderon <michal.kalde...@cavium.com>
>> ---
>> drivers/net/ethernet/qlogic/qed/qed_ll2.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> index c97ebd6..012973d 100644
>> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> @@ -201,8 +201,9 @@ void qed_ll2b_complete_rx_packet(void *cxt, struct
>> qed_ll2_comp_rx_data *data)
>>
>> skb = build_skb(buffer->data, 0);
>> if (!skb) {
>> - rc = -ENOMEM;
>> - goto out_post;
>> + DP_INFO(cdev, "Failed to build SKB\n");
>> + kfree(buffer->data);
>> + goto out_post1;
>> }
>>
>> data->u.placement_offset += NET_SKB_PAD; @@ -224,8 +225,14 @@
>> void qed_ll2b_complete_rx_packet(void *cxt, struct qed_ll2_comp_rx_data
>> *data)
>> cdev->ll2->cbs->rx_cb(cdev->ll2->cb_cookie, skb,
>> data->opaque_data_0,
>> data->opaque_data_1);
>> + } else {
>> + DP_VERBOSE(p_hwfn, (NETIF_MSG_RX_STATUS | NETIF_MSG_PKTDATA |
>> + QED_MSG_LL2 | QED_MSG_STORAGE),
>> + "Dropping the packet\n");
>> + kfree(buffer->data);
>
> What about the memory used by skb itself?
> Does skb need to be freed by kfree_skb or something like that?
>
> [Sudarsana] Thanks for reviewing the changes. qed_ll2_alloc_buffer()
> allocates this memory. The allocated buffer (i.e., buffer->data) holds
> complete memory for 'skb + data' as required by build_skb() implementation.
> Hence freeing of (buffer->data) would suffice here.
As I read through the code, the skb itself is allocated by kmem_cache_alloc,
see below:
build_skb -> __build_skb -> kmem_cache_alloc
Hope I am not missing something here.
>
>> }
>>
>> +out_post1:
>> /* Update Buffer information and update FW producer */
>> buffer->data = new_data;
>> buffer->phys_addr = new_phys_addr;
>>
>
