On Thu, Jan 28, 2021 at 9:12 AM Bob Pearson <rpearsonhpe@xxxxxxxxx> 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@xxxxxxx>
> ---
> 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
>
