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

[Cyrill Gorcunov]

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.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Andrey Vagin]

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 
> > > Signed-off-by: Cyrill Gorcunov 
> > > ---
> > >  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 

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
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Andrey Vagin]

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 
> > Signed-off-by: Cyrill Gorcunov 
> > ---
> >  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 

> > +   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

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

[Andrey Vagin]

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 
> Signed-off-by: Cyrill Gorcunov 
> ---
>  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);
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel