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

[Cyrill Gorcunov]

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

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

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

[Cyrill Gorcunov]

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);
+   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] [PATCH rh7] fs/fadvise: introduce FADV_DEACTIVATE flag

[Andrey Ryabinin]

FADV_DEACTIVATE advises kernel to move file pages from active to
inactive list.

https://jira.sw.ru/browse/PSBM-57915

Signed-off-by: Andrey Ryabinin 
---
 include/uapi/linux/fadvise.h |  1 +
 mm/fadvise.c | 43 +++
 2 files changed, 44 insertions(+)

diff --git a/include/uapi/linux/fadvise.h b/include/uapi/linux/fadvise.h
index a3e0703..b6ade7e 100644
--- a/include/uapi/linux/fadvise.h
+++ b/include/uapi/linux/fadvise.h
@@ -17,6 +17,7 @@
 #define POSIX_FADV_DONTNEED4 /* Don't need these pages.  */
 #define POSIX_FADV_NOREUSE 5 /* Data will be accessed once.  */
 #endif
+#define FADV_DEACTIVATE32 /* Mark pages as good candidates for 
reclaim */
 
 #ifdef __KERNEL__
 extern int generic_fadvise(struct file* file, loff_t off, loff_t len, int adv);
diff --git a/mm/fadvise.c b/mm/fadvise.c
index 2a1bdfb..e805d01 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -22,6 +22,43 @@
 
 #include 
 
+static void fadvise_deactivate(struct address_space *mapping,
+   pgoff_t start, pgoff_t end)
+{
+   struct pagevec pvec;
+   pgoff_t index = start;
+   int i;
+
+   if (start > end)
+   return;
+
+   /*
+* Note: this function may get called on a shmem/tmpfs mapping:
+* pagevec_lookup() might then return 0 prematurely (because it
+* got a gangful of swap entries); but it's hardly worth worrying
+* about - it can rarely have anything to free from such a mapping
+* (most pages are dirty), and already skips over any difficulties.
+*/
+
+   pagevec_init(, 0);
+   while (index <= end && pagevec_lookup(, mapping, index,
+   min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
+   for (i = 0; i < pagevec_count(); i++) {
+   struct page *page = pvec.pages[i];
+
+   /* We rely upon deletion not changing page->index */
+   index = page->index;
+   if (index > end)
+   break;
+
+   deactivate_page(page);
+   }
+   pagevec_release();
+   cond_resched();
+   index++;
+   }
+}
+
 /*
  * POSIX_FADV_WILLNEED could set PG_Referenced, and POSIX_FADV_NOREUSE could
  * deactivate the pages and clear PG_Referenced.
@@ -44,6 +81,7 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t 
len, int advice)
case POSIX_FADV_WILLNEED:
case POSIX_FADV_NOREUSE:
case POSIX_FADV_DONTNEED:
+   case FADV_DEACTIVATE:
/* no bad return value, but ignore advice */
break;
default:
@@ -124,6 +162,11 @@ int generic_fadvise(struct file *file, loff_t offset, 
loff_t len, int advice)
}
}
break;
+   case FADV_DEACTIVATE:
+   start_index = (offset+(PAGE_CACHE_SIZE-1)) >> PAGE_CACHE_SHIFT;
+   end_index = (endbyte >> PAGE_CACHE_SHIFT);
+   fadvise_deactivate(mapping, start_index, end_index);
+   break;
default:
ret = -EINVAL;
}
-- 
2.10.2

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel