* [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
@ 2021-01-28 1:12 Bob Pearson
2021-01-28 3:50 ` Zhu Yanjun
0 siblings, 1 reply; 7+ messages in thread
From: Bob Pearson @ 2021-01-28 1:12 UTC (permalink / raw)
To: jgg, zyjzyj2000, linux-rdma; +Cc: Bob Pearson
rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
code. The loop over the QPs attached to a multicast group
creates new cloned SKBs for all but the last QP in the list
and passes the SKB and its clones to rxe_rcv_pkt() for further
processing. Any QPs that do not pass some checks are skipped.
If the last QP in the list fails the tests the SKB is leaked.
This patch checks if the SKB for the last QP was used and if
not frees it. Also removes a redundant loop invariant assignment.
Fixes: 8700e3e7c4857 ("Soft RoCE driver")
Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
index c9984a28eecc..57cc25e3b4ad 100644
--- a/drivers/infiniband/sw/rxe/rxe_recv.c
+++ b/drivers/infiniband/sw/rxe/rxe_recv.c
@@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
list_for_each_entry(mce, &mcg->qp_list, qp_list) {
qp = mce->qp;
- pkt = SKB_TO_PKT(skb);
/* validate qp for incoming packet */
err = check_type_state(rxe, pkt, qp);
@@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
continue;
/* for all but the last qp create a new clone of the
- * skb and pass to the qp.
+ * skb and pass to the qp. If an error occurs in the
+ * checks for the last qp in the list we need to
+ * free the skb since it hasn't been passed on to
+ * rxe_rcv_pkt() which would free it later.
*/
- if (mce->qp_list.next != &mcg->qp_list)
+ if (mce->qp_list.next != &mcg->qp_list) {
per_qp_skb = skb_clone(skb, GFP_ATOMIC);
- else
+ } else {
per_qp_skb = skb;
+ /* show we have consumed the skb */
+ skb = NULL;
+ }
if (unlikely(!per_qp_skb))
continue;
@@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
- return;
-
err1:
- kfree_skb(skb);
+ if (skb)
+ kfree_skb(skb);
}
/**
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 1:12 [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt Bob Pearson
@ 2021-01-28 3:50 ` Zhu Yanjun
2021-01-28 3:53 ` Bob Pearson
0 siblings, 1 reply; 7+ messages in thread
From: Zhu Yanjun @ 2021-01-28 3:50 UTC (permalink / raw)
To: Bob Pearson; +Cc: Jason Gunthorpe, RDMA mailing list, Bob Pearson
On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
> code. The loop over the QPs attached to a multicast group
> creates new cloned SKBs for all but the last QP in the list
> and passes the SKB and its clones to rxe_rcv_pkt() for further
> processing. Any QPs that do not pass some checks are skipped.
> If the last QP in the list fails the tests the SKB is leaked.
> This patch checks if the SKB for the last QP was used and if
> not frees it. Also removes a redundant loop invariant assignment.
>
> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> ---
> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> index c9984a28eecc..57cc25e3b4ad 100644
> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>
> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
> qp = mce->qp;
> - pkt = SKB_TO_PKT(skb);
>
> /* validate qp for incoming packet */
> err = check_type_state(rxe, pkt, qp);
> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> continue;
>
> /* for all but the last qp create a new clone of the
> - * skb and pass to the qp.
> + * skb and pass to the qp. If an error occurs in the
> + * checks for the last qp in the list we need to
> + * free the skb since it hasn't been passed on to
> + * rxe_rcv_pkt() which would free it later.
> */
> - if (mce->qp_list.next != &mcg->qp_list)
> + if (mce->qp_list.next != &mcg->qp_list) {
> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
> - else
> + } else {
> per_qp_skb = skb;
> + /* show we have consumed the skb */
> + skb = NULL;
> + }
>
> if (unlikely(!per_qp_skb))
> continue;
> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>
> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
>
> - return;
> -
> err1:
> - kfree_skb(skb);
> + if (skb)
> + kfree_skb(skb);
"if (skb)" is not needed here.
The implemetation of kfree_skb:
void kfree_skb(struct sk_buff *skb)
{
if (unlikely(!skb))
return;
if (likely(atomic_read(&skb->users) == 1))
smp_rmb();
else if (likely(!atomic_dec_and_test(&skb->users)))
return;
trace_kfree_skb(skb, __builtin_return_address(0));
__kfree_skb(skb);
}
Zhu Yanjun
> }
>
> /**
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 3:50 ` Zhu Yanjun
@ 2021-01-28 3:53 ` Bob Pearson
2021-01-28 4:23 ` Bob Pearson
0 siblings, 1 reply; 7+ messages in thread
From: Bob Pearson @ 2021-01-28 3:53 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: Jason Gunthorpe, RDMA mailing list, Bob Pearson
On 1/27/21 9:50 PM, Zhu Yanjun wrote:
> On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
>> code. The loop over the QPs attached to a multicast group
>> creates new cloned SKBs for all but the last QP in the list
>> and passes the SKB and its clones to rxe_rcv_pkt() for further
>> processing. Any QPs that do not pass some checks are skipped.
>> If the last QP in the list fails the tests the SKB is leaked.
>> This patch checks if the SKB for the last QP was used and if
>> not frees it. Also removes a redundant loop invariant assignment.
>>
>> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
>> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>> ---
>> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
>> index c9984a28eecc..57cc25e3b4ad 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
>> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>
>> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
>> qp = mce->qp;
>> - pkt = SKB_TO_PKT(skb);
>>
>> /* validate qp for incoming packet */
>> err = check_type_state(rxe, pkt, qp);
>> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>> continue;
>>
>> /* for all but the last qp create a new clone of the
>> - * skb and pass to the qp.
>> + * skb and pass to the qp. If an error occurs in the
>> + * checks for the last qp in the list we need to
>> + * free the skb since it hasn't been passed on to
>> + * rxe_rcv_pkt() which would free it later.
>> */
>> - if (mce->qp_list.next != &mcg->qp_list)
>> + if (mce->qp_list.next != &mcg->qp_list) {
>> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
>> - else
>> + } else {
>> per_qp_skb = skb;
>> + /* show we have consumed the skb */
>> + skb = NULL;
>> + }
>>
>> if (unlikely(!per_qp_skb))
>> continue;
>> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>
>> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
>>
>> - return;
>> -
>> err1:
>> - kfree_skb(skb);
>> + if (skb)
>> + kfree_skb(skb);
>
> "if (skb)" is not needed here.
>
> The implemetation of kfree_skb:
>
> void kfree_skb(struct sk_buff *skb)
> {
> if (unlikely(!skb))
> return;
> if (likely(atomic_read(&skb->users) == 1))
> smp_rmb();
> else if (likely(!atomic_dec_and_test(&skb->users)))
> return;
> trace_kfree_skb(skb, __builtin_return_address(0));
> __kfree_skb(skb);
> }
>
> Zhu Yanjun
>> }
>>
>> /**
>> --
>> 2.27.0
>>
Agreed but the reason I wrote that was to make it obvious why I set skb to NULL above. But as long as it is clear without it I can remove the test.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 3:53 ` Bob Pearson
@ 2021-01-28 4:23 ` Bob Pearson
2021-01-28 5:00 ` Zhu Yanjun
2021-01-28 12:57 ` Leon Romanovsky
0 siblings, 2 replies; 7+ messages in thread
From: Bob Pearson @ 2021-01-28 4:23 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: Jason Gunthorpe, RDMA mailing list, Bob Pearson
On 1/27/21 9:53 PM, Bob Pearson wrote:
> On 1/27/21 9:50 PM, Zhu Yanjun wrote:
>> On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>>
>>> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
>>> code. The loop over the QPs attached to a multicast group
>>> creates new cloned SKBs for all but the last QP in the list
>>> and passes the SKB and its clones to rxe_rcv_pkt() for further
>>> processing. Any QPs that do not pass some checks are skipped.
>>> If the last QP in the list fails the tests the SKB is leaked.
>>> This patch checks if the SKB for the last QP was used and if
>>> not frees it. Also removes a redundant loop invariant assignment.
>>>
>>> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
>>> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
>>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>>> ---
>>> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
>>> index c9984a28eecc..57cc25e3b4ad 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
>>> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>>
>>> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
>>> qp = mce->qp;
>>> - pkt = SKB_TO_PKT(skb);
>>>
>>> /* validate qp for incoming packet */
>>> err = check_type_state(rxe, pkt, qp);
>>> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>> continue;
>>>
>>> /* for all but the last qp create a new clone of the
>>> - * skb and pass to the qp.
>>> + * skb and pass to the qp. If an error occurs in the
>>> + * checks for the last qp in the list we need to
>>> + * free the skb since it hasn't been passed on to
>>> + * rxe_rcv_pkt() which would free it later.
>>> */
>>> - if (mce->qp_list.next != &mcg->qp_list)
>>> + if (mce->qp_list.next != &mcg->qp_list) {
>>> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
>>> - else
>>> + } else {
>>> per_qp_skb = skb;
>>> + /* show we have consumed the skb */
>>> + skb = NULL;
>>> + }
>>>
>>> if (unlikely(!per_qp_skb))
>>> continue;
>>> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>>
>>> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
>>>
>>> - return;
>>> -
>>> err1:
>>> - kfree_skb(skb);
>>> + if (skb)
>>> + kfree_skb(skb);
>>
>> "if (skb)" is not needed here.
>>
>> The implemetation of kfree_skb:
>>
>> void kfree_skb(struct sk_buff *skb)
>> {
>> if (unlikely(!skb))
>> return;
>> if (likely(atomic_read(&skb->users) == 1))
>> smp_rmb();
>> else if (likely(!atomic_dec_and_test(&skb->users)))
>> return;
>> trace_kfree_skb(skb, __builtin_return_address(0));
>> __kfree_skb(skb);
>> }
>>
>> Zhu Yanjun
>>> }
>>>
>>> /**
>>> --
>>> 2.27.0
>>>
> Agreed but the reason I wrote that was to make it obvious why I set skb to NULL above. But as long as it is clear without it I can remove the test.
>
Actually I should have written
if (unlikely(skb))
kfree_skb(skb);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 4:23 ` Bob Pearson
@ 2021-01-28 5:00 ` Zhu Yanjun
2021-01-28 12:57 ` Leon Romanovsky
1 sibling, 0 replies; 7+ messages in thread
From: Zhu Yanjun @ 2021-01-28 5:00 UTC (permalink / raw)
To: Bob Pearson; +Cc: Jason Gunthorpe, RDMA mailing list, Bob Pearson
On Thu, Jan 28, 2021 at 12:23 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 1/27/21 9:53 PM, Bob Pearson wrote:
> > On 1/27/21 9:50 PM, Zhu Yanjun wrote:
> >> On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>>
> >>> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
> >>> code. The loop over the QPs attached to a multicast group
> >>> creates new cloned SKBs for all but the last QP in the list
> >>> and passes the SKB and its clones to rxe_rcv_pkt() for further
> >>> processing. Any QPs that do not pass some checks are skipped.
> >>> If the last QP in the list fails the tests the SKB is leaked.
> >>> This patch checks if the SKB for the last QP was used and if
> >>> not frees it. Also removes a redundant loop invariant assignment.
> >>>
> >>> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
> >>> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
> >>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> >>> ---
> >>> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
> >>> 1 file changed, 11 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> index c9984a28eecc..57cc25e3b4ad 100644
> >>> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>>
> >>> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
> >>> qp = mce->qp;
> >>> - pkt = SKB_TO_PKT(skb);
> >>>
> >>> /* validate qp for incoming packet */
> >>> err = check_type_state(rxe, pkt, qp);
> >>> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>> continue;
> >>>
> >>> /* for all but the last qp create a new clone of the
> >>> - * skb and pass to the qp.
> >>> + * skb and pass to the qp. If an error occurs in the
> >>> + * checks for the last qp in the list we need to
> >>> + * free the skb since it hasn't been passed on to
> >>> + * rxe_rcv_pkt() which would free it later.
> >>> */
> >>> - if (mce->qp_list.next != &mcg->qp_list)
> >>> + if (mce->qp_list.next != &mcg->qp_list) {
> >>> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
> >>> - else
> >>> + } else {
> >>> per_qp_skb = skb;
> >>> + /* show we have consumed the skb */
> >>> + skb = NULL;
> >>> + }
> >>>
> >>> if (unlikely(!per_qp_skb))
> >>> continue;
> >>> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>>
> >>> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
> >>>
> >>> - return;
> >>> -
> >>> err1:
> >>> - kfree_skb(skb);
> >>> + if (skb)
> >>> + kfree_skb(skb);
> >>
> >> "if (skb)" is not needed here.
> >>
> >> The implemetation of kfree_skb:
> >>
> >> void kfree_skb(struct sk_buff *skb)
> >> {
> >> if (unlikely(!skb))
> >> return;
> >> if (likely(atomic_read(&skb->users) == 1))
> >> smp_rmb();
> >> else if (likely(!atomic_dec_and_test(&skb->users)))
> >> return;
> >> trace_kfree_skb(skb, __builtin_return_address(0));
> >> __kfree_skb(skb);
> >> }
> >>
> >> Zhu Yanjun
> >>> }
> >>>
> >>> /**
> >>> --
> >>> 2.27.0
> >>>
> > Agreed but the reason I wrote that was to make it obvious why I set skb to NULL above. But as long as it is clear without it I can remove the test.
> >
> Actually I should have written
>
> if (unlikely(skb))
> kfree_skb(skb);
This is not in fast path. It is in error handling. Not sure if this
can make big optimizations.
Zhu Yanjun
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 4:23 ` Bob Pearson
2021-01-28 5:00 ` Zhu Yanjun
@ 2021-01-28 12:57 ` Leon Romanovsky
2021-01-28 17:32 ` Bob Pearson
1 sibling, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2021-01-28 12:57 UTC (permalink / raw)
To: Bob Pearson; +Cc: Zhu Yanjun, Jason Gunthorpe, RDMA mailing list, Bob Pearson
On Wed, Jan 27, 2021 at 10:23:53PM -0600, Bob Pearson wrote:
> On 1/27/21 9:53 PM, Bob Pearson wrote:
> > On 1/27/21 9:50 PM, Zhu Yanjun wrote:
> >> On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>>
> >>> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
> >>> code. The loop over the QPs attached to a multicast group
> >>> creates new cloned SKBs for all but the last QP in the list
> >>> and passes the SKB and its clones to rxe_rcv_pkt() for further
> >>> processing. Any QPs that do not pass some checks are skipped.
> >>> If the last QP in the list fails the tests the SKB is leaked.
> >>> This patch checks if the SKB for the last QP was used and if
> >>> not frees it. Also removes a redundant loop invariant assignment.
> >>>
> >>> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
> >>> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
> >>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> >>> ---
> >>> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
> >>> 1 file changed, 11 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> index c9984a28eecc..57cc25e3b4ad 100644
> >>> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> >>> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>>
> >>> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
> >>> qp = mce->qp;
> >>> - pkt = SKB_TO_PKT(skb);
> >>>
> >>> /* validate qp for incoming packet */
> >>> err = check_type_state(rxe, pkt, qp);
> >>> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>> continue;
> >>>
> >>> /* for all but the last qp create a new clone of the
> >>> - * skb and pass to the qp.
> >>> + * skb and pass to the qp. If an error occurs in the
> >>> + * checks for the last qp in the list we need to
> >>> + * free the skb since it hasn't been passed on to
> >>> + * rxe_rcv_pkt() which would free it later.
> >>> */
> >>> - if (mce->qp_list.next != &mcg->qp_list)
> >>> + if (mce->qp_list.next != &mcg->qp_list) {
> >>> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
> >>> - else
> >>> + } else {
> >>> per_qp_skb = skb;
> >>> + /* show we have consumed the skb */
> >>> + skb = NULL;
> >>> + }
> >>>
> >>> if (unlikely(!per_qp_skb))
> >>> continue;
> >>> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> >>>
> >>> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
> >>>
> >>> - return;
> >>> -
> >>> err1:
> >>> - kfree_skb(skb);
> >>> + if (skb)
> >>> + kfree_skb(skb);
> >>
> >> "if (skb)" is not needed here.
> >>
> >> The implemetation of kfree_skb:
> >>
> >> void kfree_skb(struct sk_buff *skb)
> >> {
> >> if (unlikely(!skb))
> >> return;
> >> if (likely(atomic_read(&skb->users) == 1))
> >> smp_rmb();
> >> else if (likely(!atomic_dec_and_test(&skb->users)))
> >> return;
> >> trace_kfree_skb(skb, __builtin_return_address(0));
> >> __kfree_skb(skb);
> >> }
> >>
> >> Zhu Yanjun
> >>> }
> >>>
> >>> /**
> >>> --
> >>> 2.27.0
> >>>
> > Agreed but the reason I wrote that was to make it obvious why I set skb to NULL above. But as long as it is clear without it I can remove the test.
> >
> Actually I should have written
>
> if (unlikely(skb))
> kfree_skb(skb);
Please don't put "if (a) kfree(a);" constructions unless you want to
deal with daily flux of patches with attempt to remove "if".
Thanks
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt
2021-01-28 12:57 ` Leon Romanovsky
@ 2021-01-28 17:32 ` Bob Pearson
0 siblings, 0 replies; 7+ messages in thread
From: Bob Pearson @ 2021-01-28 17:32 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Zhu Yanjun, Jason Gunthorpe, RDMA mailing list, Bob Pearson
On 1/28/21 6:57 AM, Leon Romanovsky wrote:
> On Wed, Jan 27, 2021 at 10:23:53PM -0600, Bob Pearson wrote:
>> On 1/27/21 9:53 PM, Bob Pearson wrote:
>>> On 1/27/21 9:50 PM, Zhu Yanjun wrote:
>>>> On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>>>>
>>>>> rxe_rcv_mcast_pkt() in rxe_recv.c can leak SKBs in error path
>>>>> code. The loop over the QPs attached to a multicast group
>>>>> creates new cloned SKBs for all but the last QP in the list
>>>>> and passes the SKB and its clones to rxe_rcv_pkt() for further
>>>>> processing. Any QPs that do not pass some checks are skipped.
>>>>> If the last QP in the list fails the tests the SKB is leaked.
>>>>> This patch checks if the SKB for the last QP was used and if
>>>>> not frees it. Also removes a redundant loop invariant assignment.
>>>>>
>>>>> Fixes: 8700e3e7c4857 ("Soft RoCE driver")
>>>>> Fixes: 71abf20b28ff8 ("RDMA/rxe: Handle skb_clone() failure in rxe_recv.c")
>>>>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>>>>> ---
>>>>> drivers/infiniband/sw/rxe/rxe_recv.c | 18 +++++++++++-------
>>>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
>>>>> index c9984a28eecc..57cc25e3b4ad 100644
>>>>> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
>>>>> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
>>>>> @@ -252,7 +252,6 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>>>>
>>>>> list_for_each_entry(mce, &mcg->qp_list, qp_list) {
>>>>> qp = mce->qp;
>>>>> - pkt = SKB_TO_PKT(skb);
>>>>>
>>>>> /* validate qp for incoming packet */
>>>>> err = check_type_state(rxe, pkt, qp);
>>>>> @@ -264,12 +263,18 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>>>> continue;
>>>>>
>>>>> /* for all but the last qp create a new clone of the
>>>>> - * skb and pass to the qp.
>>>>> + * skb and pass to the qp. If an error occurs in the
>>>>> + * checks for the last qp in the list we need to
>>>>> + * free the skb since it hasn't been passed on to
>>>>> + * rxe_rcv_pkt() which would free it later.
>>>>> */
>>>>> - if (mce->qp_list.next != &mcg->qp_list)
>>>>> + if (mce->qp_list.next != &mcg->qp_list) {
>>>>> per_qp_skb = skb_clone(skb, GFP_ATOMIC);
>>>>> - else
>>>>> + } else {
>>>>> per_qp_skb = skb;
>>>>> + /* show we have consumed the skb */
>>>>> + skb = NULL;
>>>>> + }
>>>>>
>>>>> if (unlikely(!per_qp_skb))
>>>>> continue;
>>>>> @@ -284,10 +289,9 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>>>>>
>>>>> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
>>>>>
>>>>> - return;
>>>>> -
>>>>> err1:
>>>>> - kfree_skb(skb);
>>>>> + if (skb)
>>>>> + kfree_skb(skb);
>>>>
>>>> "if (skb)" is not needed here.
>>>>
>>>> The implemetation of kfree_skb:
>>>>
>>>> void kfree_skb(struct sk_buff *skb)
>>>> {
>>>> if (unlikely(!skb))
>>>> return;
>>>> if (likely(atomic_read(&skb->users) == 1))
>>>> smp_rmb();
>>>> else if (likely(!atomic_dec_and_test(&skb->users)))
>>>> return;
>>>> trace_kfree_skb(skb, __builtin_return_address(0));
>>>> __kfree_skb(skb);
>>>> }
>>>>
>>>> Zhu Yanjun
>>>>> }
>>>>>
>>>>> /**
>>>>> --
>>>>> 2.27.0
>>>>>
>>> Agreed but the reason I wrote that was to make it obvious why I set skb to NULL above. But as long as it is clear without it I can remove the test.
>>>
>> Actually I should have written
>>
>> if (unlikely(skb))
>> kfree_skb(skb);
>
> Please don't put "if (a) kfree(a);" constructions unless you want to
> deal with daily flux of patches with attempt to remove "if".
>
> Thanks
>
>>
>>
Yes I get it. Thanks. -- bob
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-28 17:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 1:12 [PATCH for-next] RDMA/rxe: Fix coding error in rxe_rcv_mcast_pkt Bob Pearson
2021-01-28 3:50 ` Zhu Yanjun
2021-01-28 3:53 ` Bob Pearson
2021-01-28 4:23 ` Bob Pearson
2021-01-28 5:00 ` Zhu Yanjun
2021-01-28 12:57 ` Leon Romanovsky
2021-01-28 17:32 ` Bob Pearson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.