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

Submitted by Kirill Gorkunov on Dec. 23, 2016, 7:19 a.m.

Details

Message ID 20161223071901.GB1934@uranus
State New
Series "netlink: Don't manipulate @sk_peek_off if data fetching failed"
Headers show

Commit Message

Kirill Gorkunov Dec. 23, 2016, 7:19 a.m.
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@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@openvz.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 net/netlink/af_netlink.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

Konstantin Khorenko Feb. 6, 2017, 12:58 p.m.
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@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@openvz.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@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@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> .
>
Kirill Gorkunov Feb. 6, 2017, 1:28 p.m.
On Mon, Feb 06, 2017 at 03:58:52PM +0300, Konstantin Khorenko wrote:
> 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.

I think the patch is correct. Up to Andrew, since it's his code in first place.
Andrey Vagin March 21, 2017, 7:10 p.m.
Acked-by: Andrey Vagin <avagin@virtuozzo.com>

On Fri, Dec 23, 2016 at 10:19:01AM +0300, 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@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@openvz.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@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;
Konstantin Khorenko March 27, 2017, 12:39 p.m.
Andrey, are you going to send your patch to mainstream?
(Kirill's patch is based on your patch, AFAIK)

commit 523b88d0b87acbc7af9ae1677e993ed20f55dc5e
Author: Andrey Vagin <avagin@openvz.org>
Date:   Fri Jun 17 14:27:30 2016 +0400

     netlink: allow to set peeking offset for sockets


--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 03/21/2017 10:10 PM, Andrey Vagin wrote:
> Acked-by: Andrey Vagin <avagin@virtuozzo.com>
>
> On Fri, Dec 23, 2016 at 10:19:01AM +0300, 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@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@openvz.org>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@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@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> .
>
Andrey Vagin April 1, 2017, 1:37 a.m.
On Mon, Mar 27, 2017 at 03:39:40PM +0300, Konstantin Khorenko wrote:
> Andrey, are you going to send your patch to mainstream?
> (Kirill's patch is based on your patch, AFAIK)
> 
> commit 523b88d0b87acbc7af9ae1677e993ed20f55dc5e
> Author: Andrey Vagin <avagin@openvz.org>
> Date:   Fri Jun 17 14:27:30 2016 +0400
> 
>     netlink: allow to set peeking offset for sockets

I sent them half a year ago, but nobody answered. I will try again.
https://patchwork.criu.org/patch/628/

> 
> 
> --
> Best regards,
> 
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
> 
> On 03/21/2017 10:10 PM, Andrey Vagin wrote:
> > Acked-by: Andrey Vagin <avagin@virtuozzo.com>
> > 
> > On Fri, Dec 23, 2016 at 10:19:01AM +0300, 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@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@openvz.org>
> > > Signed-off-by: Cyrill Gorcunov <gorcunov@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@openvz.org
> > https://lists.openvz.org/mailman/listinfo/devel
> > .
> >