[Devel] [PATCH rh7 v2] netlink: Don't manipulate @sk_peek_off if data fetching failed

[Konstantin Khorenko]

So, what is the result of discussion here?

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 12/23/2016 10:19 AM, Cyrill Gorcunov wrote:
> On Fri, Dec 23, 2016 at 09:59:28AM +0300, Cyrill Gorcunov wrote:
>> On Thu, Dec 22, 2016 at 04:45:10PM -0800, Andrey Vagin wrote:
>>>
>>> Actually, this patch breaks the old behaviour even when MSG_PEEK isn't set.
>>>
>>> I was thinking a bit more and I don't understand why it is a problem. If
>>> we can't fill a buffer, an error will be returned and a user will be able
>>> to set peek_offset to get these data again.
>>
>> A user doesn't have to set peek again, without the patch the internal state
>> of sk is context-dependant, which is broken design. Take a look on unix
>> sockets code, they DON'T modify offset if something earlier failed for
>> exactly same reason.
>
> Another option -- set offset only iff data copying passed.
> ---
> From: Cyrill Gorcunov <gorcunov at openvz.org>
> Subject: [PATCH rh7 v2] netlink: Don't manipulate @sk_peek_off if data fetching failed
>
> When skb_copy_datagram_iovec called to fetch queued data
> it may fail with EFAULT and if MSG_PEEK set by a caller
> the position get advanced even if data hasn't been read.
> So we might loose data bits here on subsequent recvmsg
> calls. Instead lets exit early with error.
>
> In sake of https://jira.sw.ru/browse/PSBM-57921
>
> CC: Andrey Vagin <avagin at openvz.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  net/netlink/af_netlink.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-pcs7.git/net/netlink/af_netlink.c
> ===================================================================
> --- linux-pcs7.git.orig/net/netlink/af_netlink.c
> +++ linux-pcs7.git/net/netlink/af_netlink.c
> @@ -2473,11 +2473,12 @@ static int netlink_recvmsg(struct kiocb
>
>  	skb_reset_transport_header(data_skb);
>  	err = skb_copy_datagram_iovec(data_skb, skip, msg->msg_iov, copied);
> -
> -	if (flags & MSG_PEEK)
> -		sk_peek_offset_fwd(sk, copied);
> -	else
> -		sk_peek_offset_bwd(sk, skb->len);
> +	if (!err) {
> +		if (flags & MSG_PEEK)
> +			sk_peek_offset_fwd(sk, copied);
> +		else
> +			sk_peek_offset_bwd(sk, skb->len);
> +	}
>
>  	if (msg->msg_name) {
>  		struct sockaddr_nl *addr = (struct sockaddr_nl *)msg->msg_name;
> _______________________________________________
> Devel mailing list
> Devel at openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> .
>