netdev - Re: [Xen-devel] [PATCH net-next V7 3/4] xen-netback: coalesce slots in TX path and fix regressions

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 linux-cve-announce 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:	Tue, 30 Apr 2013 13:59:59 +0100
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Wei Liu" <wei.liu2@...rix.com>
Cc:	<ian.campbell@...rix.com>, <davem@...emloft.net>,
	<xen-devel@...ts.xen.org>, <netdev@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH net-next V7 3/4] xen-netback: coalesce
 slots in TX path and fix regressions

>>> On 22.04.13 at 14:20, Wei Liu <wei.liu2@...rix.com> wrote:
> @@ -898,47 +928,78 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
>  
>  static int netbk_count_requests(struct xenvif *vif,
>  				struct xen_netif_tx_request *first,
> +				RING_IDX first_idx,
>  				struct xen_netif_tx_request *txp,
>  				int work_to_do)
>  {
>  	RING_IDX cons = vif->tx.req_cons;

It looks bogus and confusing to me to pass first_idx into this
function when this really is the same as cons.

Jan

> -	int frags = 0;
> +	int slots = 0;
> +	int drop_err = 0;
>  
>  	if (!(first->flags & XEN_NETTXF_more_data))
>  		return 0;
>  
>  	do {
> -		if (frags >= work_to_do) {
> -			netdev_err(vif->dev, "Need more frags\n");
> +		if (slots >= work_to_do) {
> +			netdev_err(vif->dev,
> +				   "Asked for %d slots but exceeds this limit\n",
> +				   work_to_do);
>  			netbk_fatal_tx_err(vif);
>  			return -ENODATA;
>  		}
>  
> -		if (unlikely(frags >= MAX_SKB_FRAGS)) {
> -			netdev_err(vif->dev, "Too many frags\n");
> +		/* This guest is really using too many slots and
> +		 * considered malicious.
> +		 */
> +		if (unlikely(slots >= max_skb_slots)) {
> +			netdev_err(vif->dev,
> +				   "Malicious frontend using %d slots, threshold %u\n",
> +				   slots, max_skb_slots);
>  			netbk_fatal_tx_err(vif);
>  			return -E2BIG;
>  		}
>  
> -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> +		/* Xen network protocol had implicit dependency on
> +		 * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
> +		 * historical MAX_SKB_FRAGS value 18 to honor the same
> +		 * behavior as before. Any packet using more than 18
> +		 * slots but less than max_skb_slots slots is dropped
> +		 */
> +		if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
> +			if (net_ratelimit())
> +				netdev_dbg(vif->dev,
> +					   "Too many slots (%d) exceeding limit (%d), dropping packet\n",
> +					   slots, XEN_NETIF_NR_SLOTS_MIN);
> +			drop_err = -E2BIG;
> +		}
> +
> +		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
>  		       sizeof(*txp));
>  		if (txp->size > first->size) {
> -			netdev_err(vif->dev, "Frag is bigger than frame.\n");
> +			netdev_err(vif->dev,
> +				   "Invalid tx request, slot size %u > remaining size %u\n",
> +				   txp->size, first->size);
>  			netbk_fatal_tx_err(vif);
>  			return -EIO;
>  		}
>  
>  		first->size -= txp->size;
> -		frags++;
> +		slots++;
>  
>  		if (unlikely((txp->offset + txp->size) > PAGE_SIZE)) {
> -			netdev_err(vif->dev, "txp->offset: %x, size: %u\n",
> +			netdev_err(vif->dev, "Cross page boundary, txp->offset: %x, size: %u\n",
>  				 txp->offset, txp->size);
>  			netbk_fatal_tx_err(vif);
>  			return -EINVAL;
>  		}
>  	} while ((txp++)->flags & XEN_NETTXF_more_data);
> -	return frags;
> +
> +	if (drop_err) {
> +		netbk_tx_err(vif, first, first_idx + slots);
> +		return drop_err;
> +	}
> +
> +	return slots;
>  }
>  
>  static struct page *xen_netbk_alloc_page(struct xen_netbk *netbk,


--
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