Message-ID: <CA+FuTScD2vRajwg341dtyQYZs+bV2FcmD4Qh_EHR_uVwvRjbiQ@mail.gmail.com>
Date:	Thu, 4 Sep 2014 14:18:26 -0400
From:	Willem de Bruijn <willemb@...gle.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Network Development <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] sock: consistent errqueue errors and signals

On Thu, Sep 4, 2014 at 2:13 PM, Willem de Bruijn <willemb@...gle.com> wrote:
>> I was concerned that we report the error twice. Once in the sendmsg path
>> and again on recvmsg or sendmsg, which finally clears sk_err.
>
> I had overlooked that messages can be queued onto the error queue
> during send call processing (most cases of ip_local_error) as well as from
> asynchronous events (ip_icmp_error, skb_complete_tx_timestamp, ..).
>
> For the second case, the patch fixes that an enqueued message does
> not have its error returned on the first subsequent system call. By
> setting sk->sk_err unconditionally, the patch indeed introduces a
> double notification in the other case.
>
> Both paths can be made correct if sk->sk_err is set on enqueue
> only when not called from inside the send() context. The easiest
> change for timestamping is to set it in __skb_tstamp_tx directly.
> Slightly more general would be to add
>
> * /* queue errors from asynchronous events: report the error on the
> next syscall */
> + int sock_queue_err_skb_and_notify(struct sock *sk, struct sk_buff *skb)
> + {
> +  int ret = sock_queue_err_skb(sk);
> +  if (!ret)
> +    sk->sk_err = SKB_EXT_ERR(skb)->ee.ee_errno;
> + }


This was just a quick, buggy, sketch. The errno should be read and
cached locally before calling sock_queue_err_skb.

+ int ret, errno;
+
+ errno = SKB_EXT_ERR(skb)->ee.ee_errno;
+ ret = sock_queue_err_skb(sk, skb);
+ if (!ret)
+   sk->sk_err = errno;

The assignment is also not atomic and not protected by the socket
lock here. Neither is it on dequeue, but this warrants looking into.

> and call this from __skb_tstamp_tx, skb_complete_tx_timestamp,
> skb_complete_wifi_ack. It is perhaps also safe to call from
> ip[v6]_icmp_error, but as you point out there are many legacy expectations
> there, so it may be best to leave that code path as is.
>
>>> If socket option IP_RECVERR is set, the process should immediately
>>> read the error queue when a send call fails with EMSGSIZE or any other
>>> relevant error. In that case, the error is not reported more than
>>> once.
>>
>> I agree but am afraid of applications start to break.
>>
>>> The alternative patch does not overwrite sk->sk_err, but results in
>>> this same modified user-visible behavior.
>>>
>>> Without the patch, after a send call fails in the above code, the
>>> process can continue calling send/recv normally while ignoring the
>>> error, since all "if (sk->sk_err)" checks fall through. If we have to
>>> treat this as legacy behavior, then neither patch should be applied.
>>> Then, the semantics are that queued errors are non-blocking. In that
>>> case, the only patch needed for consistent semantics is to remove the
>>> sk->sk_err assigment in skb_dequeue_err_skb.
>>
>> I am afraid that we need to go down this route. :/
>>
>> I would even let the sk->sk_error_report call be handled by the protocol
>> error reporting function, as it would need to deal with sk->sk_err, too.
>>
>> Most applications susceptible to breakage here are ping and
>> tracepath/traceroute. I don't think they all check MSG_ERRQUEUE always
>> on every error. But it shouldn't matter that much because they are only
>> short running apps and error queue will be freed if application exits
>> before socket accounting will shut down the socket. If we start to
>> report one instance of an error event multiple times, I think they might
>> become confused.
>>
>>> On a related note, when returning an error, returning the errno from
>>> the item on the queue is a confusing signal. Some error codes are the
>>> result of protocol processing and have nothing queued (e.g., EINVAL),
>>> others are due to an queued error (ENOMSG) and some are simply
>>> ambiguous (EMSGSIZE). It is not reasonable to expect processes to
>>> figure out the three sets. One solution would be to dedicate a special
>>> error code to denote "there is a message queued onto sk_error_queue,
>>> read the actual errno from there". This choice only relevant if we
>>> decide to return an error, at all, of course.
>>
>> errno messages are higly sensitive regarding backwards compatibility. I
>> am totally with you that a special errno marker to indicate that a
>> packet was enqueued on the error queue would be much better. Currently
>> applications always have to query error queue on error indication,
>> otherwise socket accounting might shut down the socket at some point.
>> Sadly I don't see this change to happen.
>>
>> Bye,
>> Hannes
--
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

Powered by blists - more mailing lists