netdev - Re: [PATCH net v3] tcp: warn on bogus MSS and try to amend it

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] [day] [month] [year] [list]

Message-Id: <20161203.233703.784557738334949173.davem@davemloft.net>
Date:   Sat, 03 Dec 2016 23:37:03 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     marcelo.leitner@...il.com
Cc:     netdev@...r.kernel.org, jmaxwell37@...il.com,
        alexandre.sidorenko@....com, kuznet@....inr.ac.ru,
        jmorris@...ei.org, yoshfuji@...ux-ipv6.org, kaber@...sh.net,
        tlfalcon@...ux.vnet.ibm.com, brking@...ux.vnet.ibm.com,
        eric.dumazet@...il.com
Subject: Re: [PATCH net v3] tcp: warn on bogus MSS and try to amend it

From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Date: Fri,  2 Dec 2016 20:51:51 -0200

> @@ -144,7 +144,21 @@ static void tcp_measure_rcv_mss(struct sock *sk, const struct sk_buff *skb)
>  	 */
>  	len = skb_shinfo(skb)->gso_size ? : skb->len;
>  	if (len >= icsk->icsk_ack.rcv_mss) {
> -		icsk->icsk_ack.rcv_mss = len;
> +		static bool __once __read_mostly;
> +
> +		icsk->icsk_ack.rcv_mss = min_t(unsigned int, len,
> +					       tcp_sk(sk)->advmss);
> +		if (icsk->icsk_ack.rcv_mss != len && !__once) {
> +			struct net_device *dev;
> +
> +			__once = true;
> +
> +			rcu_read_lock();
> +			dev = dev_get_by_index_rcu(sock_net(sk), skb->skb_iif);
> +			pr_warn_once("%s: Driver has suspect GRO implementation, TCP performance may be compromised.\n",
> +				     dev ? dev->name : "Unknown driver");
> +			rcu_read_unlock();
> +		}

This is almost ready to go.

Since you are doing the 'once' logic by hand, using pr_warn_once() is
redundant.  And while you're at it, why not split this into a helper
function:

static void tcp_gro_dev_warn(struct sock *sk, const struct sk_buff *skb)
{
	static bool __once __read_mostly;

	if (!__once) {
		__once = true;

		rcu_read_lock();
		dev = dev_get_by_index_rcu(sock_net(sk), skb->skb_iif);
		pr_warn("%s: Driver has suspect GRO implementation, TCP performance may be compromised.\n",
			dev ? dev->name : "Unknown driver");
		rcu_read_unlock();
	}
}

And then call that when icsk->icsk_ack.rcv_mss != len, you can even
put an unlikely() around the condition as well.