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

Submitted by Kirill Gorkunov on Dec. 22, 2016, 3:41 p.m.

Details

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

Commit Message

Kirill Gorkunov Dec. 22, 2016, 3:41 p.m.
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 |    4 ++++
 1 file changed, 4 insertions(+)

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,6 +2473,10 @@  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 (err) {
+		skb_free_datagram(sk, skb);
+		goto out;
+	}
 
 	if (flags & MSG_PEEK)
 		sk_peek_offset_fwd(sk, copied);

Comments

Andrey Vagin Dec. 22, 2016, 11:34 p.m.
On Thu, Dec 22, 2016 at 06:41:42PM +0300, Cyrill Gorcunov wrote:
> 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 |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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,6 +2473,10 @@ 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 (err) {
> +		skb_free_datagram(sk, skb);

hmmm, why do we need to call skb_free_datagram?
> +		goto out;
> +	}
>  
>  	if (flags & MSG_PEEK)
>  		sk_peek_offset_fwd(sk, copied);
Andrey Vagin Dec. 23, 2016, 12:12 a.m.
On Thu, Dec 22, 2016 at 03:34:15PM -0800, Andrey Vagin wrote:
> On Thu, Dec 22, 2016 at 06:41:42PM +0300, Cyrill Gorcunov wrote:
> > 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 |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > 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,6 +2473,10 @@ 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 (err) {
> > +		skb_free_datagram(sk, skb);
> 
> hmmm, why do we need to call skb_free_datagram?

It should be ok, skb_free_datagram() decriments a reference counter

Acked-by: Andrey Vagin <avagin@virtuozzo.com>

> > +		goto out;
> > +	}
> >  
> >  	if (flags & MSG_PEEK)
> >  		sk_peek_offset_fwd(sk, copied);
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
Andrey Vagin Dec. 23, 2016, 12:45 a.m.
On Thu, Dec 22, 2016 at 04:12:13PM -0800, Andrey Vagin wrote:
> On Thu, Dec 22, 2016 at 03:34:15PM -0800, Andrey Vagin wrote:
> > On Thu, Dec 22, 2016 at 06:41:42PM +0300, Cyrill Gorcunov wrote:
> > > 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 |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > 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,6 +2473,10 @@ 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 (err) {
> > > +		skb_free_datagram(sk, skb);
> > 
> > hmmm, why do we need to call skb_free_datagram?
> 
> It should be ok, skb_free_datagram() decriments a reference counter
> 
> Acked-by: Andrey Vagin <avagin@virtuozzo.com>

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.

> 
> > > +		goto out;
> > > +	}
> > >  
> > >  	if (flags & MSG_PEEK)
> > >  		sk_peek_offset_fwd(sk, copied);
> > _______________________________________________
> > Devel mailing list
> > Devel@openvz.org
> > https://lists.openvz.org/mailman/listinfo/devel
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
Kirill Gorkunov Dec. 23, 2016, 6:59 a.m.
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.