netdev - Re: [PATCH net-next] sunvnet: Fixes to avoid soft lockup in sunvnet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 11 Aug 2014 13:56:42 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	sowmini.varadhan@...cle.com
Cc:	raghuram.kothakota@...cle.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] sunvnet: Fixes to avoid soft lockup in sunvnet

From: Sowmini Varadhan <sowmini.varadhan@...cle.com>
Date: Sun, 10 Aug 2014 15:48:07 -0400

> 
> 1. ldc_rx -> vnet_rx -> .. -> vnet_walk_rx->vnet_send_ack should not
>    spin into an infinite loop waiting  EAGAIN to lift.
> 
>    The sender could have sent us a burst, and gone to lunch without
>    doing any more ldc_read()'s. That should not cause the receiver to
>    loop infinitely till soft-lockup kicks in.
> 
> 2. Similarly __vnet_tx_trigger should only loop on EAGAIN a finite
>    number of times. The caller (vnet_start_xmit()) already has code
>    to reset the dring state and bail on errors from __vnet_tx_trigger()
> 
> 3. No need to ask for an ack with every vnet_start_xmit()- the single
>    ACK with DRING_STOPPED is sufficient for the protocol, and we free
>    the sk_buff in vnet_start_xmit itself, so we dont need an ACK back.
> 
> 4. At the tail of vnet_event(), if we hit the maybe_tx_wakeup()
>    condition, we try to take the netif_tx_lock() in the
>    recv-interrupt-context and can deadlock with dev_watchdog().
>    Two changes to fix this:
>    (a) The contents of maybe_tx_wakeup() should be moved into
>        vnet_tx_timeout
>    (b) vnet_event() should kick off a tasklet that triggers
>        netif_wake_queue()
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> Acked-by: Raghuram Kothakota <raghuram.kothakota@...cle.com>

Please don't fix multiple things in one huge patch, split up the
changes into separate easily reviewed patches.

> +/*
> + * Heuristic for the number of times to exponentially backoff and
> + * retry sending an LDC trigger when EAGAIN is encountered
> + */

In the networking, comments are formatted:

/* Like
 * this.
 */

> +			printk(KERN_INFO 
> +			    "ECONNRESET %x:%x:%x:%x:%x:%x\n",
> +			    port->raddr[0], port->raddr[1],
> +			    port->raddr[2], port->raddr[3],
> +			    port->raddr[4], port->raddr[5]);

This is not indented correctly.  The second and subsequent lines
of the printk() function call should start at exactly the first
column after the openning parenthesis of the function call.

> +	/*
> +	 * Kick off a tasklet to wake the queue.  We cannot call 
> +	 * maybe_tx_wakeup directly here because we could deadlock on
> +	 * netif_tx_lock() with dev_watchdog()
> +	 */

Comment formatting.

> +	/*
> +	 * We don't rely on the ACKs to free the skb in vnet_start_xmit(),
> +	 * thus it is safe to not set VIO_ACK_ENABLE for each transmission:
> +	 * the protocol itself does not require it as long as the peer
> +	 * sends a VIO_SUBTYPE_ACK for VIO_DRING_STOPPED. 
> +	 *
> +	 * An ACK for every packet in the ring is expensive as the
> +	 * sending of LDC messages is slow and affects performance.
> +	 */

Likewise.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html