Re: [PATCH net-next] sock: consistent errqueue errors and signals

[Hannes Frederic Sowa <hannes@redhat.com>]

On So, 2014-08-31 at 21:28 -0400, Willem de Bruijn wrote:
> When a socket error is pending, send()/recv() must abort their normal
> operation and return the error. An error means having non-zero
> sk->sk_err or having non-empty sk->sk_error_queue.
> 
> Currently, the behavior for the second is inconsistent depending on
> whether an error has previously been dequeued. In all cases,
> recv()/send() test sk->sk_err. This is not modified on enqueue onto
> the error queue, so may be 0. It is modified on dequeue, however, to
> match the queued skb's errno. I observed the following when two errors
> were queued:
> 
>   ret = poll(pollfd, 1, -1);
>   assert(ret == 1);
>   assert(pollfd.revents == POLLERR);
> 
>   ret = recv(fd, buf, size, MSG_NONBLOCK);
>   assert(ret == -1 && errno == EAGAIN);		/* <-- A */
> 
>   ret = recv(fd, buf, size, MSG_ERRQUEUE);
>   assert(ret > 0);
> 
>   ret = recv(fd, buf, size, MSG_NONBLOCK);
>   assert(ret == -1 && errno == ENOMSG);		/* <-- B */
> 
>   ret = recv(fd, buf, size, MSG_ERRQUEUE);
>   assert(ret > 0);
> 
> The recv call in B returns the error code embedded in
> SKB_EXT_ERR(skb), in this case ENOMSG, because I am working with
> timestamps. The recv call in A should have returned the
> same.
> 
> Implement this behavior. This may surprise existing applications.
> 
> Also make the wake-up signal when data is ready on the error queue
> consistent between enqueue and dequeue: use sk_error_report in both
> cases.
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> ---
> 
> This approach leaves one issue:
> The states of sk->sk_err and sk->sk_error_queue are related, but only
> loosely. Error queue enqueue, dequeue and other code may overwrite
> sk->sk_err unconditionally. For one, sock_error will reset
> sk->sk_err to 0 even if sk->sk_error_queue is not empty. If socket
> calls should abort on all errors, then should be change to test
> sk_error_queue.qlen. But, doing so requires taking a lock in a busy
> data path.
> ---
>  net/core/skbuff.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 163b673..f7a280b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3485,8 +3485,11 @@ int sock_queue_err_skb(struct sock *sk, struct sk_buff *skb)
>  	skb_dst_force(skb);
>  
>  	skb_queue_tail(&sk->sk_error_queue, skb);
> +	sk->sk_err = SKB_EXT_ERR(skb)->ee.ee_errno;
> +
>  	if (!sock_flag(sk, SOCK_DEAD))
> -		sk->sk_data_ready(sk);
> +		sk->sk_error_report(sk);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(sock_queue_err_skb);

>From my experience in IPv6 code, we only do sk->sk_err updates directly
in protocol error handling code. In case of UDP IPv6 errors for example
we now notify sk_error_report two times with this patch (before the
patch we did sk_data_ready (this is what you changed) and
sk_error_report).

I really wonder if setting sk->sk_err in this function is the right
thing to do. It also depends on socket state bits (e.g. np->recverr) if
the update happens. So we still cannot get rid of the protocol dependent
sk->sk_err updates.

It looks like we have to check all error handling functions in the
protocols. Maybe timestamp code needs to adapt?

Thanks,
Hannes