netdev - Re: [PATCHv3] netem: Segment GSO packets on enqueue

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1461953366.5535.152.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Fri, 29 Apr 2016 11:09:26 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
	"David S. Miller" <davem@...emloft.net>,
	netem@...ts.linux-foundation.org
Subject: Re: [PATCHv3] netem: Segment GSO packets on enqueue

On Fri, 2016-04-29 at 13:35 -0400, Neil Horman wrote:
> This was recently reported to me, and reproduced on the latest net kernel, when
> attempting to run netperf from a host that had a netem qdisc attached to the
> egress interface:
> 


>  /*
>   * Insert one skb into qdisc.
>   * Note: parent depends on return value to account for queue length.
> @@ -407,7 +426,11 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  	/* We don't fill cb now as skb_unshare() may invalidate it */
>  	struct netem_skb_cb *cb;
>  	struct sk_buff *skb2;
> +	struct sk_buff *segs = NULL;
> +	unsigned int len = 0, prev_len = qdisc_pkt_len(skb);
> +	int nb = 0;
>  	int count = 1;
> +	int rc = NET_XMIT_SUCCESS;
>  
>  	/* Random duplication */
>  	if (q->duplicate && q->duplicate >= get_crandom(&q->dup_cor))
> @@ -453,10 +476,22 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  	 * do it now in software before we mangle it.
>  	 */
>  	if (q->corrupt && q->corrupt >= get_crandom(&q->corrupt_cor)) {
> +		if (skb_is_gso(skb)) {
> +			segs = netem_segment(skb, sch);
> +			if (!segs)
> +				return NET_XMIT_DROP;
> +		} else
> +			segs = skb;
> +
> +		skb = segs;
> +		segs = segs->next;
> +
>  		if (!(skb = skb_unshare(skb, GFP_ATOMIC)) ||
>  		    (skb->ip_summed == CHECKSUM_PARTIAL &&
> -		     skb_checksum_help(skb)))
> -			return qdisc_drop(skb, sch);
> +		     skb_checksum_help(skb))) {
> +			rc = qdisc_drop(skb, sch);
> +			goto finish_segs;
> +		}
>  
>  		skb->data[prandom_u32() % skb_headlen(skb)] ^=
>  			1<<(prandom_u32() % 8);
> @@ -516,7 +551,26 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  		sch->qstats.requeues++;
>  	}
>  
> -	return NET_XMIT_SUCCESS;
> +finish_segs:
> +	if (segs) {
> +		while (segs) {
> +			skb2 = segs->next;
> +			segs->next = NULL;
> +			qdisc_skb_cb(segs)->pkt_len = segs->len;
> +			len += segs->len;

Wrong operation if packet is dropped by qdisc_enqueue() ?

I would use
 u32 last_len = segs->len;

> +			rc = qdisc_enqueue(segs, sch);
> +			if (rc != NET_XMIT_SUCCESS) {
> +				if (net_xmit_drop_count(rc))
> +					qdisc_qstats_drop(sch);
> +			} else

   } else {
        nb++;
        len += last_len;
   }

> +				nb++;
> +			segs = skb2;
> +		}
> +		sch->q.qlen += nb;
> +		if (nb > 1)
> +			qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
> +	}
> +	return rc;

Seems you should return NET_XMIT_SUCCESS instead of status of last
segment ?

>  }

Thanks.