[PATCH/RFC] net: Don't save mid batch datagram processing error for next recvmmsg call

[PATCH/RFC] net: Don't save mid batch datagram processing error for next recvmmsg call

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

Hi,

	I think this addresses the problems reported by David Laight and
others, where errors saved on a per socket area could be delivered to a
different thread, so I just followed David Laight's suggestion and
stopped saving it, we'll return it only if it happens for the first
datagram, else we return less entries than asked for.

	Steven, IIRC you was the one that suggested using this
mechanism, no? Do you have anything against this move?

- Arnaldo


[-- Attachment #2: 0002-net-Don-t-save-mid-batch-datagram-processing-error-f.patch --]
[-- Type: text/plain, Size: 2635 bytes --]

>From 5cf1a5c682c8bb0add9adfb5167a63f269136523 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Mon, 21 Jul 2014 12:39:56 -0300
Subject: [PATCH 2/2] net: Don't save mid batch datagram processing error for
 next recvmmsg call
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When the call to the underlying per protocol recvmsg fails for some
reason and we already have successfully processed at least one datagram
in the batch, we were saving the error for the next recvmmsg (or
recvmsg) call, so the user would eventually get notified of what
happened.

This was found to be fraught with problems, as, for instance, errors
could be notified for a different thread than a faulty one providing
invalid buffers.

So just return the number of datagrams successfully received.

Reported-by: David Laight <David.Laight@ACULAB.COM>
Cc: Caitlin Bestler <caitlin.bestler@gmail.com>
Cc: Chris Friesen <chris.friesen@windriver.com>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Elie De Brauwer <eliedebrauwer@gmail.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Ondřej Bílka <neleai@seznam.cz>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Rémi Denis-Courmont <remi@remlab.net>
Cc: Steven Whitehouse <steve@chygwyn.com>
Link: http://lkml.kernel.org/n/net-next-yfgcm0gwo7un6eqf89xw47lo@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 net/socket.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 310a50971769..b6836c5fd201 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2436,28 +2436,13 @@ out_put:
 		timeout->tv_nsec = (timeout_hz % HZ) * (NSEC_PER_SEC / HZ);
 	}
 
-	if (err == 0)
-		return datagrams;
-
-	if (datagrams != 0) {
-		/*
-		 * We may return less entries than requested (vlen) if the
-		 * sock is non block and there aren't enough datagrams...
-		 */
-		if (err != -EAGAIN) {
-			/*
-			 * ... or  if recvmsg returns an error after we
-			 * received some datagrams, where we record the
-			 * error to return on the next call or if the
-			 * app asks about it using getsockopt(SO_ERROR).
-			 */
-			sock->sk->sk_err = -err;
-		}
-
-		return datagrams;
-	}
-
-	return err;
+	/*
+	 * We may return less entries than requested (vlen) if the
+	 * sock is non block and there aren't enough datagrams or
+	 * if some problem happened after successfully receiving one
+	 * datagram.
+	 */
+	return datagrams ?: err;
 }
 
 SYSCALL_DEFINE5(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,
-- 
1.9.3

Re: [PATCH/RFC] net: Don't save mid batch datagram processing error for next recvmmsg call

[David Miller]

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Mon, 21 Jul 2014 16:30:42 -0300

> From 5cf1a5c682c8bb0add9adfb5167a63f269136523 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Mon, 21 Jul 2014 12:39:56 -0300
> Subject: [PATCH 2/2] net: Don't save mid batch datagram processing error for
>  next recvmmsg call
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> When the call to the underlying per protocol recvmsg fails for some
> reason and we already have successfully processed at least one datagram
> in the batch, we were saving the error for the next recvmmsg (or
> recvmsg) call, so the user would eventually get notified of what
> happened.
> 
> This was found to be fraught with problems, as, for instance, errors
> could be notified for a different thread than a faulty one providing
> invalid buffers.
> 
> So just return the number of datagrams successfully received.
> 
> Reported-by: David Laight <David.Laight@ACULAB.COM>
 ...
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

This is absolutely correct, any time we have a partial success we must
indicate success and the amount that was properly transferred, leaving
the error signalling for a subsequent retry (if any).

RE: [PATCH/RFC] net: Don't save mid batch datagram processing error for next recvmmsg call

[David Laight]

From: Arnaldo Carvalho de Melo 
> 	I think this addresses the problems reported by David Laight and
> others, where errors saved on a per socket area could be delivered to a
> different thread, so I just followed David Laight's suggestion and
> stopped saving it, we'll return it only if it happens for the first
> datagram, else we return less entries than asked for.
> 
> 	Steven, IIRC you was the one that suggested using this
> mechanism, no? Do you have anything against this move?

+	return datagrams ?: err;

Inline patches, don't attach them.

Don't use non-C constructs.

	David

Re: [PATCH/RFC] net: Don't save mid batch datagram processing error for next recvmmsg call

['Arnaldo Carvalho de Melo']

Em Tue, Jul 22, 2014 at 08:33:27AM +0000, David Laight escreveu:
> From: Arnaldo Carvalho de Melo 
> > 	I think this addresses the problems reported by David Laight and
> > others, where errors saved on a per socket area could be delivered to a
> > different thread, so I just followed David Laight's suggestion and
> > stopped saving it, we'll return it only if it happens for the first
> > datagram, else we return less entries than asked for.

> > 	Steven, IIRC you was the one that suggested using this
> > mechanism, no? Do you have anything against this move?
 
> +	return datagrams ?: err;
 
> Inline patches, don't attach them.

Oh well, ok, will do next time.
 
> Don't use non-C constructs.

We use it in many places, one could say the kernel isn't written in C
anyway:

[acme@zoo linux]$ find . -name "*.[ch]" | xargs grep '?:' | cut -d'/' -f -2 | sort | uniq -c | sort -nr
    189 ./drivers
     45 ./arch
     33 ./crypto
     30 ./fs
     25 ./net
     23 ./kernel
     22 ./tools
      9 ./sound
      8 ./include
      5 ./security
      5 ./mm
      3 ./block
      2 ./lib
      1 ./scripts
      1 ./ipc
[acme@zoo linux]$

Other than that, are you ok with the change?

- Arnaldo