On Fri, Aug 24, 2018 at 08:42:46AM -0400, Aaron Knister wrote:
> Inside of start_xmit() the call to check if the connection is up and the
> queueing of the packets for later transmission is not atomic which
> leaves a window where cm_rep_handler can run, set the connection up,
> dequeue pending packets and leave the subsequently queued packets by
> start_xmit() sitting on neigh->queue until they're dropped when the
> connection is torn down. This only applies to connected mode. These
> dropped packets can really upset TCP, for example, and cause
> multi-minute delays in transmission for open connections.
>
> Here's the code in start_xmit where we check to see if the connection
> is up:
>
> if (ipoib_cm_get(neigh)) {
> if (ipoib_cm_up(neigh)) {
> ipoib_cm_send(dev, skb, ipoib_cm_get(neigh));
> goto unref;
> }
> }
>
> The race occurs if cm_rep_handler execution occurs after the above
> connection check (specifically if it gets to the point where it acquires
> priv->lock to dequeue pending skb's) but before the below code snippet
> in start_xmit where packets are queued.
>
> if (skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE) {
> push_pseudo_header(skb, phdr->hwaddr);
> spin_lock_irqsave(&priv->lock, flags);
> __skb_queue_tail(&neigh->queue, skb);
> spin_unlock_irqrestore(&priv->lock, flags);
> } else {
> ++dev->stats.tx_dropped;
> dev_kfree_skb_any(skb);
> }
>
> The patch acquires the netif tx lock in cm_rep_handler for the section
> where it sets the connection up and dequeues and retransmits deferred
> skb's.
>
> Signed-off-by: Aaron Knister <aaron.s.knister@xxxxxxxx>
Tested-by: Ira Weiny <ira.weiny@xxxxxxxxx>
Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index 6535d9be..a620701f 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -1028,12 +1028,14 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id, struct ib_cm_event *even
>
> skb_queue_head_init(&skqueue);
>
> + netif_tx_lock_bh(p->dev);
> spin_lock_irq(&priv->lock);
> set_bit(IPOIB_FLAG_OPER_UP, &p->flags);
> if (p->neigh)
> while ((skb = __skb_dequeue(&p->neigh->queue)))
> __skb_queue_tail(&skqueue, skb);
> spin_unlock_irq(&priv->lock);
> + netif_tx_unlock_bh(p->dev);
>
> while ((skb = __skb_dequeue(&skqueue))) {
> skb->dev = p->dev;
> --
> 2.12.3
>
