netdev - Re: [PATCH net-next] sock: do not set sk_err in sock_dequeue_err

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]

Message-ID: <a3f7ea70-658e-9836-1a09-d1b9586582eb@stressinduktion.org>
Date:   Fri, 4 Nov 2016 00:10:55 +0100
From:   Hannes Frederic Sowa <hannes@...essinduktion.org>
To:     Soheil Hassas Yeganeh <soheil.kdev@...il.com>, davem@...emloft.net,
        netdev@...r.kernel.org
Cc:     edumazet@...gle.com, willemb@...gle.com, ncardwell@...gle.com,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH net-next] sock: do not set sk_err in sock_dequeue_err_skb

[also cc'ed Andy, albeit this doesn't seem to solve his initial problem,
right? <http://www.spinics.net/lists/netdev/msg331753.html>]

On 03.11.2016 23:24, Soheil Hassas Yeganeh wrote:
> From: Soheil Hassas Yeganeh <soheil@...gle.com>
> 
> Do not set sk_err when dequeuing errors from the error queue.
> Doing so results in:
> a) Bugs: By overwriting existing sk_err values, it possibly
>    hides legitimate errors. It is also incorrect when local
>    errors are queued with ip_local_error. That happens in the
>    context of a system call, which already returns the error
>    code.
> b) Inconsistent behavior: When there are pending errors on
>    the error queue, sk_err is sometimes 0 (e.g., for
>    the first timestamp on the error queue) and sometimes
>    set to an error code (after dequeuing the first
>    timestamp).
> c) Suboptimality: Setting sk_err to ENOMSG on simple
>    TX timestamps can abort parallel reads and writes.
> 
> Removing this line doesn't break userspace. This is because
> userspace code cannot rely on sk_err for detecting whether
> there is something on the error queue. Except for ICMP messages
> received for UDP and RAW, sk_err is not set at enqueue time,
> and as a result sk_err can be 0 while there are plenty of
> errors on the error queue.
> 
> For ICMP packets in UDP and RAW, sk_err is set when they are
> enqueued on the error queue, but that does not result in aborting
> reads and writes. For such cases, sk_err is only readable via
> getsockopt(SO_ERROR) which will reset the value of sk_err on
> its own. More importantly, prior to this patch,
> recvmsg(MSG_ERRQUEUE) has a race on setting sk_err (i.e.,
> sk_err is set by sock_dequeue_err_skb without atomic ops or
> locks) which can store 0 in sk_err even when we have ICMP
> messages pending. Removing this line from sock_dequeue_err_skb
> eliminates that race.
> 
> Signed-off-by: Soheil Hassas Yeganeh <soheil@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> Signed-off-by: Neal Cardwell <ncardwell@...gle.com>

I think it makes sense to remove this given your argumentation.

Acked-by: Hannes Frederic Sowa <hannes@...essinduktion.org>