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

[Andrey Vagin]

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 at 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 at 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 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
> > .
> >