| Message ID | 20180619045802.24050-2-sudarsana.kalluru@cavium.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | qed*: Fix series. | expand |
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.Kalluru@cavium.com> > Signed-off-by: Ariel Elior <ariel.elior@cavium.com> > Signed-off-by: Michal Kalderon <Michal.Kalderon@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? > } > > +out_post1: > /* Update Buffer information and update FW producer */ > buffer->data = new_data; > buffer->phys_addr = new_phys_addr; >
-----Original Message----- From: Yunsheng Lin [mailto:linyunsheng@huawei.com] Sent: 19 June 2018 11:32 To: Kalluru, Sudarsana <Sudarsana.Kalluru@cavium.com>; davem@davemloft.net Cc: netdev@vger.kernel.org; Elior, Ariel <Ariel.Elior@cavium.com>; Kalderon, Michal <Michal.Kalderon@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.Kalluru@cavium.com> > Signed-off-by: Ariel Elior <ariel.elior@cavium.com> > Signed-off-by: Michal Kalderon <Michal.Kalderon@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. > } > > +out_post1: > /* Update Buffer information and update FW producer */ > buffer->data = new_data; > buffer->phys_addr = new_phys_addr; >
On 2018/6/19 14:42, Kalluru, Sudarsana wrote: > > > -----Original Message----- > From: Yunsheng Lin [mailto:linyunsheng@huawei.com] > Sent: 19 June 2018 11:32 > To: Kalluru, Sudarsana <Sudarsana.Kalluru@cavium.com>; davem@davemloft.net > Cc: netdev@vger.kernel.org; Elior, Ariel <Ariel.Elior@cavium.com>; Kalderon, Michal <Michal.Kalderon@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.Kalluru@cavium.com> >> Signed-off-by: Ariel Elior <ariel.elior@cavium.com> >> Signed-off-by: Michal Kalderon <Michal.Kalderon@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; >> >
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); } +out_post1: /* Update Buffer information and update FW producer */ buffer->data = new_data; buffer->phys_addr = new_phys_addr;
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.Kalluru@cavium.com> Signed-off-by: Ariel Elior <ariel.elior@cavium.com> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> --- drivers/net/ethernet/qlogic/qed/qed_ll2.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)