Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Matthias Andree]

(Please Cc: Frans and myself on replies, we're not subscribed to netdev@.)

(DaveM, this was Cc:d to you earlier today under a different subject. The  
failing wget is new though.)

Dear Kernel Hackers,

I am getting rather frequent "TCP(...): Application bug, race in  
MSG_PEEK." complaints for fetchmail these days.
The strange part is that fetchmail is a single-threaded application that  
does not deal with URG data.
I cannot see what fetchmail would race against in this situation.

Frans Pop has found interesting patterns probably related to segment or  
packet/payload sizes (1460 bytes!) - see his message quoted below.

His most recent findings are:
: As further info, I've also got very infrequent MSGPEEK errors on my
: laptop, which runs nothing strange (... KDE ...)
: Interesting may be that both systems are dual core, running x86_64.
:
: Here are the errors from my laptop (also with my extra debug info):
: Mar 14 09:23:18 aragorn kernel: TCP(kio_imap4:5646): Application bug,  
race in MSG_PEEK: 0, 23.
: Apr  8 12:21:28 aragorn kernel: TCP(kio_imap4:24173): Application bug,  
race in MSG_PEEK: 38dd5ab8, 8.
: May  3 23:45:29 aragorn kernel: TCP(wget:4137): Application bug, race in  
MSG_PEEK: 77093ab8, 3e.
: Note the last one. A simple wget?!

Application use: Fetchmail does use MSG_PEEK in several places to look  
ahead. The same process that peeks will later read the data. It can happen  
that we peek at 1 byte, then more bytes, or that we peek at 512 bytes and  
read only smaller amounts (say, 30 - 70) of them before we peek again.

As to the application source code: Look for instance for fm_peek in the  
function SockRead() in  
<http://mknod.org/svn/fetchmail/branches/BRANCH_6-3/socket.c> and its  
callers.

Can we be sure that

(a) the kernel correctly handles peek_seq and tp->copied_seq and their  
comparison (see Frans's message below - is the != in the if statement  
actually the right thing or should this be a > or <?), and

(b) that the message is printed only if there is a real app bug (that I  
fail to see), and

(c) that the kernel handles package boundaries, wraparounds, and buffers  
correctly?

Might the reception of further data in the socket's receive buffer trigger  
the message? As in: between peek and read, or between read and peek,  
further data arrives, and triggers the message?

Might the kernel's TCP code be confused about absolute vs. relative  
sequence/position indexes/pointers/counters inside the TCP code?

Thanks a lot in advance. (Frans's earlier message included below.)

Best regards
Matthias


------- Weitergeleitete Nachricht -------
Von: "Frans Pop" <elendil@planet.nl>
An: "Matthias Andree" <matthias.andree@gmx.de>
Kopie:
Betreff: Re: Bug#513695: fetchmail: race in MSG_PEEK - trace data
Datum: Sat, 14 Mar 2009 19:00:13 +0100

On Monday 09 March 2009, Matthias Andree wrote:
> Unfortunately, the strace seems to be incomplete - likely strace macros
> wrong: I see lots of recvfrom with replies, but not the send or write
> that actually shows the other half of the conversation. Could I have
> coordinated full strace logging and kernel messages?

See attachment. The syslog is complete for the period the tcpdump was
running, but the tcpdump itself is filtered for only the relevant
conversation. Again account name and password removed from the traces.

It took a bit longer because I wanted to get some history from a kernel
patch I added. I've now automated the creation of the traces, so if you
want more data, even from the kernel, please ask.

I've applied the following patch to net/ipv4/tcp.c for the kernel running
on my server (2.6.29-rc8):
@@ -1499,8 +1499,9 @@ do_prequeue:
     		}
     		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
     			if (net_ratelimit())
-				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
-				       current->comm, task_pid_nr(current));
+				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK: %x,
%x.\n",
+				       current->comm, task_pid_nr(current)),
+				       peek_seq, tp->copied_seq;
     			peek_seq = tp->copied_seq;
     		}
     		continue;

So, the values you see at the end of the warning are peek_seq and
tp->copied_seq. This gives messages like:
kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 156233,
16a.
kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 71259ac8,
5b4.
kernel: TCP(fetchmail:31216): Application bug, race in MSG_PEEK: 833fe5,
c0.

The distribution for tp->copied_seq is interesting:
          1 24
          1 c0
          1 132
          1 16a
          3 248
          1 2a7
          1 400
        166 5b4

So, 0x5b4 (1460) seems to be special (is it a maximum size maybe?); if
I grep the kernel for that I get:
net/ipv4/tcp_input.c:           if (tp->mss_cache > 1460)
net/ipv4/tcp_output.c:          if (mss > 1460 * 3)
net/ipv4/tcp_output.c:          else if (mss > 1460)
And:
net/ipv[46]/syncookies.c:
static __u16 const msstab[] = {
            64 - 1,
            256 - 1,
            512 - 1,
            536 - 1,
            1024 - 1,
            1440 - 1,
            1460 - 1,
            4312 - 1,
            (__u16)-1
};

Not sure if all that means anything, but I thought I'd mention it.

Good hunting,
Frans



-- 
Matthias Andree

Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Matthias Andree]

(Please Cc: Frans and myself on replies, we're not subscribed to netdev@.)
(Resent from mutt, after Opera trashed my headers...)
(DaveM, this was Cc:d to you earlier today under a different subject. The  
failing wget is new though.)

Dear Kernel Hackers,

I am getting rather frequent "TCP(...): Application bug, race in  
MSG_PEEK." complaints for fetchmail these days.
The strange part is that fetchmail is a single-threaded application that  
does not deal with URG data.
I cannot see what fetchmail would race against in this situation.

Frans Pop has found interesting patterns probably related to segment or  
packet/payload sizes (1460 bytes!) - see his message quoted below.

His most recent findings are:
: As further info, I've also got very infrequent MSGPEEK errors on my
: laptop, which runs nothing strange (... KDE ...)
: Interesting may be that both systems are dual core, running x86_64.
:
: Here are the errors from my laptop (also with my extra debug info):
: Mar 14 09:23:18 aragorn kernel: TCP(kio_imap4:5646): Application bug,  
race in MSG_PEEK: 0, 23.
: Apr  8 12:21:28 aragorn kernel: TCP(kio_imap4:24173): Application bug,  
race in MSG_PEEK: 38dd5ab8, 8.
: May  3 23:45:29 aragorn kernel: TCP(wget:4137): Application bug, race in  
MSG_PEEK: 77093ab8, 3e.
: Note the last one. A simple wget?!

Application use: Fetchmail does use MSG_PEEK in several places to look  
ahead. The same process that peeks will later read the data. It can happen  
that we peek at 1 byte, then more bytes, or that we peek at 512 bytes and  
read only smaller amounts (say, 30 - 70) of them before we peek again.

As to the application source code: Look for instance for fm_peek in the  
function SockRead() in  
<http://mknod.org/svn/fetchmail/branches/BRANCH_6-3/socket.c> and its  
callers.

Can we be sure that

(a) the kernel correctly handles peek_seq and tp->copied_seq and their  
comparison (see Frans's message below - is the != in the if statement  
actually the right thing or should this be a > or <?), and

(b) that the message is printed only if there is a real app bug (that I  
fail to see), and

(c) that the kernel handles package boundaries, wraparounds, and buffers  
correctly?

Might the reception of further data in the socket's receive buffer trigger  
the message? As in: between peek and read, or between read and peek,  
further data arrives, and triggers the message?

Might the kernel's TCP code be confused about absolute vs. relative  
sequence/position indexes/pointers/counters inside the TCP code?

Thanks a lot in advance. (Frans's earlier message included below.)

Best regards
Matthias


------- Weitergeleitete Nachricht -------
Von: "Frans Pop" <elendil@planet.nl>
An: "Matthias Andree" <matthias.andree@gmx.de>
Kopie:
Betreff: Re: Bug#513695: fetchmail: race in MSG_PEEK - trace data
Datum: Sat, 14 Mar 2009 19:00:13 +0100

On Monday 09 March 2009, Matthias Andree wrote:
> Unfortunately, the strace seems to be incomplete - likely strace macros
> wrong: I see lots of recvfrom with replies, but not the send or write
> that actually shows the other half of the conversation. Could I have
> coordinated full strace logging and kernel messages?

See attachment. The syslog is complete for the period the tcpdump was
running, but the tcpdump itself is filtered for only the relevant
conversation. Again account name and password removed from the traces.

It took a bit longer because I wanted to get some history from a kernel
patch I added. I've now automated the creation of the traces, so if you
want more data, even from the kernel, please ask.

I've applied the following patch to net/ipv4/tcp.c for the kernel running
on my server (2.6.29-rc8):
@@ -1499,8 +1499,9 @@ do_prequeue:
     		}
     		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
     			if (net_ratelimit())
-				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
-				       current->comm, task_pid_nr(current));
+				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK: %x,
%x.\n",
+				       current->comm, task_pid_nr(current)),
+				       peek_seq, tp->copied_seq;
     			peek_seq = tp->copied_seq;
     		}
     		continue;

So, the values you see at the end of the warning are peek_seq and
tp->copied_seq. This gives messages like:
kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 156233,
16a.
kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 71259ac8,
5b4.
kernel: TCP(fetchmail:31216): Application bug, race in MSG_PEEK: 833fe5,
c0.

The distribution for tp->copied_seq is interesting:
          1 24
          1 c0
          1 132
          1 16a
          3 248
          1 2a7
          1 400
        166 5b4

So, 0x5b4 (1460) seems to be special (is it a maximum size maybe?); if
I grep the kernel for that I get:
net/ipv4/tcp_input.c:           if (tp->mss_cache > 1460)
net/ipv4/tcp_output.c:          if (mss > 1460 * 3)
net/ipv4/tcp_output.c:          else if (mss > 1460)
And:
net/ipv[46]/syncookies.c:
static __u16 const msstab[] = {
            64 - 1,
            256 - 1,
            512 - 1,
            536 - 1,
            1024 - 1,
            1440 - 1,
            1460 - 1,
            4312 - 1,
            (__u16)-1
};

Not sure if all that means anything, but I thought I'd mention it.

Good hunting,
Frans



-- 
Matthias Andree

Re: Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7275 bytes --]

On Thu, 7 May 2009, Matthias Andree wrote:

> (Please Cc: Frans and myself on replies, we're not subscribed to netdev@.)
> (Resent from mutt, after Opera trashed my headers...)
> (DaveM, this was Cc:d to you earlier today under a different subject. The
> failing wget is new though.)
> 
> Dear Kernel Hackers,
> 
> I am getting rather frequent "TCP(...): Application bug, race in MSG_PEEK."
> complaints for fetchmail these days.
> The strange part is that fetchmail is a single-threaded application that does
> not deal with URG data.
> I cannot see what fetchmail would race against in this situation.
> 
> Frans Pop has found interesting patterns probably related to segment or
> packet/payload sizes (1460 bytes!) - see his message quoted below.
> 
> His most recent findings are:
> : As further info, I've also got very infrequent MSGPEEK errors on my
> : laptop, which runs nothing strange (... KDE ...)
> : Interesting may be that both systems are dual core, running x86_64.
> :
> : Here are the errors from my laptop (also with my extra debug info):
> : Mar 14 09:23:18 aragorn kernel: TCP(kio_imap4:5646): Application bug, race
> : in MSG_PEEK: 0, 23.
> : Apr  8 12:21:28 aragorn kernel: TCP(kio_imap4:24173): Application bug, race
> : in MSG_PEEK: 38dd5ab8, 8.
> : May  3 23:45:29 aragorn kernel: TCP(wget:4137): Application bug, race in
> : MSG_PEEK: 77093ab8, 3e.
> : Note the last one. A simple wget?!
> 
> Application use: Fetchmail does use MSG_PEEK in several places to look ahead.
> The same process that peeks will later read the data. It can happen that we
> peek at 1 byte, then more bytes, or that we peek at 512 bytes and read only
> smaller amounts (say, 30 - 70) of them before we peek again.
> 
> As to the application source code: Look for instance for fm_peek in the
> function SockRead() in
> <http://mknod.org/svn/fetchmail/branches/BRANCH_6-3/socket.c> and its callers.
> 
> Can we be sure that
> 
> (a) the kernel correctly handles peek_seq and tp->copied_seq and their
> comparison (see Frans's message below - is the != in the if statement actually
> the right thing or should this be a > or <?), and
> 
> (b) that the message is printed only if there is a real app bug (that I fail
> to see), and
> 
> (c) that the kernel handles package boundaries, wraparounds, and buffers
> correctly?
> 
> Might the reception of further data in the socket's receive buffer trigger the
> message? As in: between peek and read, or between read and peek, further data
> arrives, and triggers the message?
> 
> Might the kernel's TCP code be confused about absolute vs. relative
> sequence/position indexes/pointers/counters inside the TCP code?
> 
> Thanks a lot in advance. (Frans's earlier message included below.)
> 
> Best regards
> Matthias
> 
> 
> ------- Weitergeleitete Nachricht -------
> Von: "Frans Pop" <elendil@planet.nl>
> An: "Matthias Andree" <matthias.andree@gmx.de>
> Kopie:
> Betreff: Re: Bug#513695: fetchmail: race in MSG_PEEK - trace data
> Datum: Sat, 14 Mar 2009 19:00:13 +0100
> 
> On Monday 09 March 2009, Matthias Andree wrote:
> >Unfortunately, the strace seems to be incomplete - likely strace macros
> >wrong: I see lots of recvfrom with replies, but not the send or write
> >that actually shows the other half of the conversation. Could I have
> >coordinated full strace logging and kernel messages?
> 
> See attachment. The syslog is complete for the period the tcpdump was
> running, but the tcpdump itself is filtered for only the relevant
> conversation. Again account name and password removed from the traces.
> 
> It took a bit longer because I wanted to get some history from a kernel
> patch I added. I've now automated the creation of the traces, so if you
> want more data, even from the kernel, please ask.
> 
> I've applied the following patch to net/ipv4/tcp.c for the kernel running
> on my server (2.6.29-rc8):
> @@ -1499,8 +1499,9 @@ do_prequeue:
>    		}
>    		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
>    			if (net_ratelimit())
> -				printk(KERN_DEBUG "TCP(%s:%d): Application
> bug, race in MSG_PEEK.\n",
> -				       current->comm, task_pid_nr(current));
> +				printk(KERN_DEBUG "TCP(%s:%d): Application
> bug, race in MSG_PEEK: %x,
> %x.\n",
> +				       current->comm, task_pid_nr(current)),
> +				       peek_seq, tp->copied_seq;

I cannot resist myself from noting that this certainly wasn't the patch 
one got those printks below... It might happily compile though :-).

>    			peek_seq = tp->copied_seq;
>    		}
>    		continue;
> 
> So, the values you see at the end of the warning are peek_seq and
> tp->copied_seq. This gives messages like:
> kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 156233,
> 16a.
> kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 71259ac8,
> 5b4.
> kernel: TCP(fetchmail:31216): Application bug, race in MSG_PEEK: 833fe5,
> c0.
> 
> The distribution for tp->copied_seq is interesting:
>         1 24
>         1 c0
>         1 132
>         1 16a
>         3 248
>         1 2a7
>         1 400
>       166 5b4
> 
> So, 0x5b4 (1460) seems to be special (is it a maximum size maybe?); if

1460 is size of the maximum wire segment when not using timestamps (1500 
MTU - (20+20 headers), would be 1448 with timestamps). So basically the 
skbs in the receiver queue will be of that length (holds until the driver 
of your nic is converted to gro).

> I grep the kernel for that I get:
> net/ipv4/tcp_input.c:           if (tp->mss_cache > 1460)
> net/ipv4/tcp_output.c:          if (mss > 1460 * 3)
> net/ipv4/tcp_output.c:          else if (mss > 1460)
> And:
> net/ipv[46]/syncookies.c:
> static __u16 const msstab[] = {
>           64 - 1,
>           256 - 1,
>           512 - 1,
>           536 - 1,
>           1024 - 1,
>           1440 - 1,
>           1460 - 1,
>           4312 - 1,
>           (__u16)-1
> };
> 
> Not sure if all that means anything, but I thought I'd mention it.

What would you think about the following, untested patch... I suppose it 
is enough to capture the racy situations except with that crazy urg hole, 
grr (I suppose that will need just another variable to do the offset
of one).

-- 
 i.

[RFC PATCH] tcp: fix MSG_PEEK race check

Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
blocking behavior) lets the loop run longer than this check
did previously expect, so we need to be more careful with
this check and consider the work we have been doing.

I'm a bit unsure if this improved check can still fail as
	if (!sock_flag(sk, SOCK_URGINLINE)) {
		++*seq;
		...
does not increment copied.

Compile tested.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1d7f49c..ccbd69b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1532,7 +1532,7 @@ do_prequeue:
 				}
 			}
 		}
-		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
+		if ((flags & MSG_PEEK) && (peek_seq - copied != tp->copied_seq)) {
 			if (net_ratelimit())
 				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
 				       current->comm, task_pid_nr(current));
-- 
tg: (4d5b78c..) fix/msgpeek-race-chk (depends on: origin/master)

Re: Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Frans Pop]

On Thursday 07 May 2009, Ilpo Järvinen wrote:
> On Thu, 7 May 2009, Matthias Andree wrote:
> > I've applied the following patch to net/ipv4/tcp.c for the kernel
> > running on my server (2.6.29-rc8):
> > @@ -1499,8 +1499,9 @@ do_prequeue:
> >    		}
> >    		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
> >    			if (net_ratelimit())
> > -				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
> > -				       current->comm, task_pid_nr(current));
> > +				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK: %x, %x.\n",
> > +				       current->comm, task_pid_nr(current)),
> > +				       peek_seq, tp->copied_seq;
>
> I cannot resist myself from noting that this certainly wasn't the patch
> one got those printks below... It might happily compile though :-).

Can you please elaborate why you think that? It may be horribly broken
(I've never claimed to be a C coder, and probably never will), but it
also really is the patch that generates the printks...

> >    			peek_seq = tp->copied_seq;
> >    		}
> >    		continue;
> >
> > So, the values you see at the end of the warning are peek_seq and
> > tp->copied_seq. This gives messages like:
> > kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 156233, 16a.
> > kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 71259ac8, 5b4.
> > kernel: TCP(fetchmail:31216): Application bug, race in MSG_PEEK: 833fe5, c0.

[...]

> What would you think about the following, untested patch... I suppose
> it is enough to capture the racy situations except with that crazy urg
> hole, grr (I suppose that will need just another variable to do the
> offset of one).

I'll give your patch a try and report back.

Thanks,
FJP

Re: Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2173 bytes --]

On Thu, 7 May 2009, Frans Pop wrote:

> On Thursday 07 May 2009, Ilpo Järvinen wrote:
> > On Thu, 7 May 2009, Matthias Andree wrote:
> > > I've applied the following patch to net/ipv4/tcp.c for the kernel
> > > running on my server (2.6.29-rc8):
> > > @@ -1499,8 +1499,9 @@ do_prequeue:
> > >    		}
> > >    		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
> > >    			if (net_ratelimit())
> > > -				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
> > > -				       current->comm, task_pid_nr(current));
> > > +				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK: %x, %x.\n",
> > > +				       current->comm, task_pid_nr(current)),
> > > +				       peek_seq, tp->copied_seq;
> >
> > I cannot resist myself from noting that this certainly wasn't the patch
> > one got those printks below... It might happily compile though :-).
> 
> Can you please elaborate why you think that? It may be horribly broken
> (I've never claimed to be a C coder, and probably never will), but it
> also really is the patch that generates the printks...

...This was mainly meant to be a joke... :-)

The parenthesis won't match how a printk with string and 4 args should 
be called, so with this version you have in the mail peek_seq and 
tp->copied_seq are not put into stack or you were just super lucky.

> > >    			peek_seq = tp->copied_seq;
> > >    		}
> > >    		continue;
> > >
> > > So, the values you see at the end of the warning are peek_seq and
> > > tp->copied_seq. This gives messages like:
> > > kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 156233, 16a.
> > > kernel: TCP(fetchmail:9311): Application bug, race in MSG_PEEK: 71259ac8, 5b4.
> > > kernel: TCP(fetchmail:31216): Application bug, race in MSG_PEEK: 833fe5, c0.
> 
> [...]
> 
> > What would you think about the following, untested patch... I suppose
> > it is enough to capture the racy situations except with that crazy urg
> > hole, grr (I suppose that will need just another variable to do the
> > offset of one).
> 
> I'll give your patch a try and report back.

Thanks. If it works I can add that urg hole madness handling too there.

-- 
 i.

Re: Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Frans Pop]

On Thursday 07 May 2009, Ilpo Järvinen wrote:
> > Can you please elaborate why you think that? It may be horribly
> > broken (I've never claimed to be a C coder, and probably never will),
> > but it also really is the patch that generates the printks...
>
> ...This was mainly meant to be a joke... :-)
>
> The parenthesis won't match how a printk with string and 4 args should
> be called, so with this version you have in the mail peek_seq and
> tp->copied_seq are not put into stack or you were just super lucky.

Ah, yes. You're absolutely right. Duh.
gcc does warn about it, but I must have missed that.

Re: Strange Application bug, race in MSG_PEEK complaints (was: Bug#513695: fetchmail: race in MSG_PEEK)

[Frans Pop]

On Thursday 07 May 2009, Ilpo Järvinen wrote:
> [RFC PATCH] tcp: fix MSG_PEEK race check
>
> Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> blocking behavior) lets the loop run longer than this check
> did previously expect, so we need to be more careful with
> this check and consider the work we have been doing.
>
> I'm a bit unsure if this improved check can still fail as
>         if (!sock_flag(sk, SOCK_URGINLINE)) {
>                 ++*seq;
>                 ...
> does not increment copied.
>
> Compile tested.
>
> Signed-off-by: Ilpo J?rvinen <ilpo.jarvinen@helsinki.fi>

I've been running with the patch for 2 days now and have not seen any more 
MSG_PEEK errors, so as far as I'm concerned the patch does fix the issue 
(needed the time in order to be confident of that).

So:
Tested-by: Frans Pop <elendil@planet.nl>

Suggest to also add a CC for stable.

Cheers,
FJP

[PATCH v2] tcp: fix MSG_PEEK race check

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2731 bytes --]

On Sat, 9 May 2009, Frans Pop wrote:

> On Thursday 07 May 2009, Ilpo Järvinen wrote:
> > [RFC PATCH] tcp: fix MSG_PEEK race check
> >
> > Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> > blocking behavior) lets the loop run longer than this check
> > did previously expect, so we need to be more careful with
> > this check and consider the work we have been doing.
> >
> > I'm a bit unsure if this improved check can still fail as
> >         if (!sock_flag(sk, SOCK_URGINLINE)) {
> >                 ++*seq;
> >                 ...
> > does not increment copied.
> >
> > Compile tested.
> >
> > Signed-off-by: Ilpo J?rvinen <ilpo.jarvinen@helsinki.fi>
> 
> I've been running with the patch for 2 days now and have not seen any more 
> MSG_PEEK errors, so as far as I'm concerned the patch does fix the issue 
> (needed the time in order to be confident of that).
>
> So:
> Tested-by: Frans Pop <elendil@planet.nl>

Added.

> Suggest to also add a CC for stable.

DaveM will take care of stable submissions by himself.


I took my time to fix the urg_hole madness too. The patch below.

--
[PATCH v2] tcp: fix MSG_PEEK race check

Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
blocking behavior) lets the loop run longer than the race check
did previously expect, so we need to be more careful with this
check and consider the work we have been doing.

I tried my best to deal with urg hole madness too which happens
here:
	if (!sock_flag(sk, SOCK_URGINLINE)) {
		++*seq;
		...
by using additional offset by one but I certainly have very
little interest in testing that part.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Tested-by: Frans Pop <elendil@planet.nl>
---
 net/ipv4/tcp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1d7f49c..7a0f0b2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1321,6 +1321,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	struct task_struct *user_recv = NULL;
 	int copied_early = 0;
 	struct sk_buff *skb;
+	u32 urg_hole = 0;
 
 	lock_sock(sk);
 
@@ -1532,7 +1533,8 @@ do_prequeue:
 				}
 			}
 		}
-		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
+		if ((flags & MSG_PEEK) &&
+		    (peek_seq - copied - urg_hole != tp->copied_seq)) {
 			if (net_ratelimit())
 				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
 				       current->comm, task_pid_nr(current));
@@ -1553,6 +1555,7 @@ do_prequeue:
 				if (!urg_offset) {
 					if (!sock_flag(sk, SOCK_URGINLINE)) {
 						++*seq;
+						urg_hole++;
 						offset++;
 						used--;
 						if (!used)
-- 
tg: (4d5b78c..) fix/msgpeek-race-chk (depends on: origin/master)

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Frans Pop]

On Monday 11 May 2009, Ilpo Järvinen wrote:
> I took my time to fix the urg_hole madness too. The patch below.

Hmm. I wonder if it wouldn't be better to keep the two issues separate. 
The initial patch is a clear regression fix (4 people have reported it 
against fetchmail for Debian). The URG part is IMO a separate issue which 
I at least have never seen in practice.
And my Tested-by doesn't cover the additional change either.

That said, I have added the URG change (as an incremental patch) in my 
local git repo and will give it a go when I next build a kernel (may take 
a week). I don't expect to be able to confirm it fixes the URG race, but 
I can at least check that it doesn't cause any false messages with my 
(spectacularly unspectacular) network traffic. I'll report the result.

> --
> [PATCH v2] tcp: fix MSG_PEEK race check
>
> Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> blocking behavior) lets the loop run longer than the race check
> did previously expect, so we need to be more careful with this
> check and consider the work we have been doing.
>
> I tried my best to deal with urg hole madness too which happens
> here:
> 	if (!sock_flag(sk, SOCK_URGINLINE)) {
> 		++*seq;
> 		...
> by using additional offset by one but I certainly have very
> little interest in testing that part.
>
> Signed-off-by: Ilpo J?rvinen <ilpo.jarvinen@helsinki.fi>
> Tested-by: Frans Pop <elendil@planet.nl>
> ---
>  net/ipv4/tcp.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1d7f49c..7a0f0b2 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1321,6 +1321,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock
> *sk, struct msghdr *msg, struct task_struct *user_recv = NULL;
>  	int copied_early = 0;
>  	struct sk_buff *skb;
> +	u32 urg_hole = 0;
>
>  	lock_sock(sk);
>
> @@ -1532,7 +1533,8 @@ do_prequeue:
>  				}
>  			}
>  		}
> -		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
> +		if ((flags & MSG_PEEK) &&
> +		    (peek_seq - copied - urg_hole != tp->copied_seq)) {
>  			if (net_ratelimit())
>  				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in
> MSG_PEEK.\n", current->comm, task_pid_nr(current));
> @@ -1553,6 +1555,7 @@ do_prequeue:
>  				if (!urg_offset) {
>  					if (!sock_flag(sk, SOCK_URGINLINE)) {
>  						++*seq;
> +						urg_hole++;
>  						offset++;
>  						used--;
>  						if (!used)

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2407 bytes --]

On Mon, 11 May 2009, Frans Pop wrote:

> On Monday 11 May 2009, Ilpo Järvinen wrote:
> > I took my time to fix the urg_hole madness too. The patch below.
> 
> Hmm. I wonder if it wouldn't be better to keep the two issues separate. 
> The initial patch is a clear regression fix (4 people have reported it 
> against fetchmail for Debian). The URG part is IMO a separate issue which 
> I at least have never seen in practice.
> And my Tested-by doesn't cover the additional change either.

Disagreed. It's true that your testing very likely doesn't cover such a 
corner case. The URG thing is legacy which shouldn't exist anymore but it 
might still be that some people are crazy enough to use URG not inline 
(and at the same time are doing MSG_PEEK too). However, that URG part is
not a _separate_ issue, you might not just have a test case but it happens 
due to the very same reason and was broken by the very same commit.

This issue has nothing to do with fetchmail or so alone (regardless of how 
many bugs have been filed against it), it's generic TCP (in kernel) issue, 
whether it's triggered is just about right test pattern which here happens 
with fetchmail but it is by no means limited to it.

I don't care too much if distro people have some local policies regarding 
fixes and that here shouldn't be a bother to them anyway since there's the 
more limited fix available in the archives too if they specifically want 
that.

> That said, I have added the URG change (as an incremental patch) in my 
> local git repo and will give it a go when I next build a kernel (may take 
> a week). I don't expect to be able to confirm it fixes the URG race, but 
> I can at least check that it doesn't cause any false messages with my 
> (spectacularly unspectacular) network traffic.

There's no URG race you're implying! There's peek_seq != tp->copied_seq 
race check which can currently trigger spuriously because of more looping 
that what used to be done. Thus each adjustment of peek_seq (through *seq) 
in the loop must be countered by opposite adjustment for the purpose of 
the check. This urg_hole just covers the other of the two cases there are
to adjust *seq (the other is countered by the -copied part).

If you don't have URG holes, the v2 change yields to: -0 which equals to 
no-op. No testing is going to undo that :-). ...And that can be seen
already from the patch context.


-- 
 i.

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Frans Pop]

On Monday 11 May 2009, Ilpo Järvinen wrote:
> On Mon, 11 May 2009, Frans Pop wrote:
> > On Monday 11 May 2009, Ilpo Järvinen wrote:
> > > I took my time to fix the urg_hole madness too. The patch below.
> >
> > Hmm. I wonder if it wouldn't be better to keep the two issues
> > separate. The initial patch is a clear regression fix (4 people have
> > reported it against fetchmail for Debian). The URG part is IMO a
> > separate issue which I at least have never seen in practice.
> > And my Tested-by doesn't cover the additional change either.
>
> Disagreed. It's true that your testing very likely doesn't cover such a
> corner case. The URG thing is legacy which shouldn't exist anymore but
> it might still be that some people are crazy enough to use URG not
> inline (and at the same time are doing MSG_PEEK too). However, that URG
> part is not a _separate_ issue, you might not just have a test case but
> it happens due to the very same reason and was broken by the very same
> commit.

OK. I understood that there's always been a corner case with URG that 
could cause incorrect messages [1] and I thought the additional change 
was to fix that, but if this is related to the same regression then of 
course it's fine by me.

[1] http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-09/6009.html

> This issue has nothing to do with fetchmail or so alone (regardless of
> how many bugs have been filed against it), it's generic TCP (in kernel)
> issue, whether it's triggered is just about right test pattern which
> here happens with fetchmail but it is by no means limited to it.

I never claimed that. In fact, I was the one who also saw the issue with 
other applications (wget, IMAP)...

> I don't care too much if distro people have some local policies
> regarding fixes and that here shouldn't be a bother to them anyway
> since there's the more limited fix available in the archives too if
> they specifically want that.

Where did that come from? Not from anything I said...

> If you don't have URG holes, the v2 change yields to: -0 which equals
> to no-op. No testing is going to undo that :-). ...And that can be seen
> already from the patch context.

I'll still give the new patch a try on my next build :-)
The rest I happily leave up to you and David.

Thanks for clarifying.

Cheers,
FJP

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2905 bytes --]

On Mon, 11 May 2009, Frans Pop wrote:

> On Monday 11 May 2009, Ilpo Järvinen wrote:
> > On Mon, 11 May 2009, Frans Pop wrote:
> > > On Monday 11 May 2009, Ilpo Järvinen wrote:
> > > > I took my time to fix the urg_hole madness too. The patch below.
> > >
> > > Hmm. I wonder if it wouldn't be better to keep the two issues
> > > separate. The initial patch is a clear regression fix (4 people have
> > > reported it against fetchmail for Debian). The URG part is IMO a
> > > separate issue which I at least have never seen in practice.
> > > And my Tested-by doesn't cover the additional change either.
> >
> > Disagreed. It's true that your testing very likely doesn't cover such a
> > corner case. The URG thing is legacy which shouldn't exist anymore but
> > it might still be that some people are crazy enough to use URG not
> > inline (and at the same time are doing MSG_PEEK too). However, that URG
> > part is not a _separate_ issue, you might not just have a test case but
> > it happens due to the very same reason and was broken by the very same
> > commit.
> 
> OK. I understood that there's always been a corner case with URG that 
> could cause incorrect messages [1] and I thought the additional change 
> was to fix that, but if this is related to the same regression then of 
> course it's fine by me.
> 
> [1] http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-09/6009.html

Ah, so there's some urg race for real... ...I didn't know about that
which is no wonder since I've very little interest on knowing all
corner cases of urg madness really :-).

I guess it is not exactly the same though I have problem in understanding 
what Dave is exactly meaning there but it could well be that there isn't 
a sane case where the urg hole thing matters for real. Well, Dave probably 
knows whether the v2 is necessary or not, I've no clue who is the one 
advancing the copied_seq (if it's not the gem in tcp_check_urg doing the 
conditional copied_seq++, but that condition is beyond my current level of 
concentration really).

> > This issue has nothing to do with fetchmail or so alone (regardless of
> > how many bugs have been filed against it), it's generic TCP (in kernel)
> > issue, whether it's triggered is just about right test pattern which
> > here happens with fetchmail but it is by no means limited to it.
> 
> I never claimed that. In fact, I was the one who also saw the issue with 
> other applications (wget, IMAP)...
> 
> > I don't care too much if distro people have some local policies
> > regarding fixes and that here shouldn't be a bother to them anyway
> > since there's the more limited fix available in the archives too if
> > they specifically want that.
> 
> Where did that come from? Not from anything I said...

Hmm... Referring to some distro bug reports to strengthen the separate 
problems argument caused the impression, I'm sorry about that :-).


-- 
 i.

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 11 May 2009 17:57:13 +0300 (EEST)

> On Mon, 11 May 2009, Frans Pop wrote:
> 
>> On Monday 11 May 2009, Ilpo Järvinen wrote:
>> OK. I understood that there's always been a corner case with URG that 
>> could cause incorrect messages [1] and I thought the additional change 
>> was to fix that, but if this is related to the same regression then of 
>> course it's fine by me.
>> 
>> [1] http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-09/6009.html
> 
> Ah, so there's some urg race for real... ...I didn't know about that
> which is no wonder since I've very little interest on knowing all
> corner cases of urg madness really :-).
> 
> I guess it is not exactly the same though I have problem in understanding 
> what Dave is exactly meaning there but it could well be that there isn't 
> a sane case where the urg hole thing matters for real. Well, Dave probably 
> knows whether the v2 is necessary or not, I've no clue who is the one 
> advancing the copied_seq (if it's not the gem in tcp_check_urg doing the 
> conditional copied_seq++, but that condition is beyond my current level of 
> concentration really).

The issue being discussed there is exactly the case where a thread
is triggering the copied_seq advance in tcp_check_urg() and using
MSG_PEEK at the same time.

I'm looking more closely into this patch right now, but I might ask
you to split the two fixes up if I can't convince myself %100 of the
URG part.  We've already broken URG enough lately :-)

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 11 May 2009 09:32:34 +0300 (EEST)

> [PATCH v2] tcp: fix MSG_PEEK race check
> 
> Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> blocking behavior) lets the loop run longer than the race check
> did previously expect, so we need to be more careful with this
> check and consider the work we have been doing.
> 
> I tried my best to deal with urg hole madness too which happens
> here:
> 	if (!sock_flag(sk, SOCK_URGINLINE)) {
> 		++*seq;
> 		...
> by using additional offset by one but I certainly have very
> little interest in testing that part.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> Tested-by: Frans Pop <elendil@planet.nl>

Ok, now that I've looked at this, the urg_hole part of this change has
to be removed.

That case being accounted for with urg_hole is exactly what the
debugging message is trying to catch, where we are doing MSG_PEEK and
tcp_check_urg() advances ->copied_seq on us during one of those
"release_sock();/lock_sock();" sequences (which thus invoke TCP input
processing).

Could you please respin this patch with the URG bits removed?

Thanks!

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3580 bytes --]

On Sun, 17 May 2009, David Miller wrote:

> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 11 May 2009 09:32:34 +0300 (EEST)
> 
> > [PATCH v2] tcp: fix MSG_PEEK race check
> > 
> > Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> > blocking behavior) lets the loop run longer than the race check
> > did previously expect, so we need to be more careful with this
> > check and consider the work we have been doing.
> > 
> > I tried my best to deal with urg hole madness too which happens
> > here:
> > 	if (!sock_flag(sk, SOCK_URGINLINE)) {
> > 		++*seq;
> > 		...
> > by using additional offset by one but I certainly have very
> > little interest in testing that part.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> > Tested-by: Frans Pop <elendil@planet.nl>
> 
> Ok, now that I've looked at this, the urg_hole part of this change has
> to be removed.

Thanks for taking a look... :-) The first patch btw was with RFC and 
without urg bits btw but you put that into some discarded like(?) state in 
patchwork... :-/

> That case being accounted for with urg_hole is exactly what the
> debugging message is trying to catch, where we are doing MSG_PEEK and
> tcp_check_urg() advances ->copied_seq on us during one of those
> "release_sock();/lock_sock();" sequences (which thus invoke TCP input
> processing).

Sorry, I'm not sure you thought this fully through here. What my patch 
with urg_hole does is that it keeps peek_seq non-advancing using the 
offsets. Now without the urg offsetting, the peek_seq side of the race 
check advances, which means that we didn't catch the race when it happened 
for real as copied_seq advanced too?!?

>From another perspective when the race didn't happen but potential for it 
existed, the current check should catch that since peek_seq advanced and 
copied_seq stayed where it was. But that certainly doesn't match to your 
description above.

I wonder why all this fuzz about this particular race as we will do our 
best anyway to skip the hole on MSG_PEEK side (if I read the code right)? 
Either we never see the hole in MSG_PEEK side, or we're happily past it 
already, does it make any difference anymore in the latter case? <insert 
some "not that I fully understand all of that multipage function" 
disclaimer here though, I may think too simple scenarios here>. Not that 
I'm too interested in "improving" urg or so anywa, I'm just curious... :-)

> Could you please respin this patch with the URG bits removed?

Below.

-- 
 i.

--
[PATCH] tcp: fix MSG_PEEK race check

Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
blocking behavior) lets the loop run longer than the race check
did previously expect, so we need to be more careful with this
check and consider the work we have been doing.

URG hole things postponed to another patch (if needed at all)
as per requested.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Tested-by: Frans Pop <elendil@planet.nl>

---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1d7f49c..ccbd69b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1532,7 +1532,7 @@ do_prequeue:
 				}
 			}
 		}
-		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
+		if ((flags & MSG_PEEK) && (peek_seq - copied != tp->copied_seq)) {
 			if (net_ratelimit())
 				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n",
 				       current->comm, task_pid_nr(current));
-- 
tg: (4d5b78c..) fix/msgpeek-race-chk (depends on: origin/master)

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Matthias Andree]

Am 18.05.2009, 00:31 Uhr, schrieb David Miller <davem@davemloft.net>:

> The issue being discussed there is exactly the case where a thread
> is triggering the copied_seq advance in tcp_check_urg() and using
> MSG_PEEK at the same time.
>
> I'm looking more closely into this patch right now, but I might ask
> you to split the two fixes up if I can't convince myself %100 of the
> URG part.  We've already broken URG enough lately :-)

As long as the non-URG part goes into 2.6.30 (it's a regression fix in my  
perception), I'll be a happy camper - at least that would stop the  
kernel's finger pointing at innocent applications. 8-)

OK, I presume an application is innocent if it doesn't use MSG_OOB flags -  
is that sufficient?

-- 
Matthias Andree

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Matthias Andree]

Am 18.05.2009, 09:24 Uhr, schrieb Ilpo Järvinen  
<ilpo.jarvinen@helsinki.fi>:

> On Sun, 17 May 2009, David Miller wrote:
>
>> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
>> Date: Mon, 11 May 2009 09:32:34 +0300 (EEST)
>>
>> > [PATCH v2] tcp: fix MSG_PEEK race check
>> >
>> > Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
>> > blocking behavior) lets the loop run longer than the race check
>> > did previously expect, so we need to be more careful with this
>> > check and consider the work we have been doing.
>> >
>> > I tried my best to deal with urg hole madness too which happens
>> > here:
>> > 	if (!sock_flag(sk, SOCK_URGINLINE)) {
>> > 		++*seq;
>> > 		...
>> > by using additional offset by one but I certainly have very
>> > little interest in testing that part.
>> >
>> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
>> > Tested-by: Frans Pop <elendil@planet.nl>
>>
>> Ok, now that I've looked at this, the urg_hole part of this change has
>> to be removed.
>
> Thanks for taking a look... :-) The first patch btw was with RFC and
> without urg bits btw but you put that into some discarded like(?) state  
> in patchwork... :-/

WRT the earlier patch, we have one more success report from one of the  
users who reported the problem, namely Ian Zimmermann:

<http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=513695#155>

-- 
Matthias Andree

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 18 May 2009 10:24:23 +0300 (EEST)

> Sorry, I'm not sure you thought this fully through here. What my patch 
> with urg_hole does is that it keeps peek_seq non-advancing using the 
> offsets. Now without the urg offsetting, the peek_seq side of the race 
> check advances, which means that we didn't catch the race when it happened 
> for real as copied_seq advanced too?!?

I see now what the situation is, thanks for explaining.

You're account for the "*seq" advance for URG that happens in
tcp_recvmsg() rather than the one that happens in tcp_check_urg().

> From another perspective when the race didn't happen but potential for it 
> existed, the current check should catch that since peek_seq advanced and 
> copied_seq stayed where it was. But that certainly doesn't match to your 
> description above.

Right.

> I wonder why all this fuzz about this particular race as we will do our 
> best anyway to skip the hole on MSG_PEEK side (if I read the code right)? 
> Either we never see the hole in MSG_PEEK side, or we're happily past it 
> already, does it make any difference anymore in the latter case? <insert 
> some "not that I fully understand all of that multipage function" 
> disclaimer here though, I may think too simple scenarios here>. Not that 
> I'm too interested in "improving" urg or so anywa, I'm just curious... :-)

A long long time ago we had an assertion here checking whether
->copied_seq moved without our knowledge.  Alexey and I found this
could trigger and investigation of that is what helped us
find the tcp_check_urg()+MSG_PEEK case.  That's when we added this
test and log message.

When the MSG_PEEK test existing in the !copied if() branch of
tcp_recvmsg(), so many of these code paths we are dealing with in this
fix could simply not be reached when MSG_PEEK.  That ++*seq could
never happen, because if "copied" was non-zero and MSG_PEEK was true
we would leave the loop and return from tcp_recvmsg() immediately.

Now, we have to accomodate those adjustments.

Since I now understand your urg_hole code I'm going to apply your v2
patch which takes care of the URG stuff.  I also buy the argument that
perhaps we should get rid of the log message, but look at how it
helped us find this kernel regression :-)

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4537 bytes --]

On Mon, 18 May 2009, David Miller wrote:

> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 18 May 2009 10:24:23 +0300 (EEST)
> 
> > Sorry, I'm not sure you thought this fully through here. What my patch 
> > with urg_hole does is that it keeps peek_seq non-advancing using the 
> > offsets. Now without the urg offsetting, the peek_seq side of the race 
> > check advances, which means that we didn't catch the race when it happened 
> > for real as copied_seq advanced too?!?
> 
> I see now what the situation is, thanks for explaining.
> 
> You're account for the "*seq" advance for URG that happens in
> tcp_recvmsg() rather than the one that happens in tcp_check_urg().

Right.

[I've moved the next fragment here from below...]

> When the MSG_PEEK test existing in the !copied if() branch of
> tcp_recvmsg(), so many of these code paths we are dealing with in this
> fix could simply not be reached when MSG_PEEK.  That ++*seq could
> never happen, because if "copied" was non-zero and MSG_PEEK was true
> we would leave the loop and return from tcp_recvmsg() immediately.
>
> Now, we have to accomodate those adjustments.

...I thought I wrote something along those lines in the log message 
(though I failed to make it so dead obvious as you do it here :-)).
 
> > From another perspective when the race didn't happen but potential for it 
> > existed, the current check should catch that since peek_seq advanced and 
> > copied_seq stayed where it was. But that certainly doesn't match to your 
> > description above.
> 
> Right.

[...more moving of fragments here...]

> Since I now understand your urg_hole code I'm going to apply your v2
> patch which takes care of the URG stuff.

I wonder if you realized, that after my change the check _no longer_ 
catches this case where the potential exists but race didn't happen for 
real? (bleh, I realized while writing the previous mail that this might be 
misinterpreted but I was lazy enough to not fix that :-()

If we want to "forbid" urgs during msg_peek (ie., unconditionally warn), 
we could just change the check to do || urg_hole and that would catch both 
cases, or even better, just clone it into if (MSG_PEEK) under urg hole 
branch. The latter would even allow us to distinguish between the cases => 
more intelligent message can then be given in the urg case but that of 
course needs first the decision that it's something we must check for
at all.

> > I wonder why all this fuzz about this particular race as we will do our 
> > best anyway to skip the hole on MSG_PEEK side (if I read the code right)? 
> > Either we never see the hole in MSG_PEEK side, or we're happily past it 
> > already, does it make any difference anymore in the latter case? <insert 
> > some "not that I fully understand all of that multipage function" 
> > disclaimer here though, I may think too simple scenarios here>. Not that 
> > I'm too interested in "improving" urg or so anywa, I'm just curious... :-)
> 
> A long long time ago we had an assertion here checking whether
> ->copied_seq moved without our knowledge.  Alexey and I found this
> could trigger and investigation of that is what helped us
> find the tcp_check_urg()+MSG_PEEK case.  That's when we added this
> test and log message.

Sure, the copied_seq is moving, but what I'm after is that does it really 
make any difference from tcp_recvmsg point of view? It certainly triggers 
the message but that won't work as a proof of the evilness for me.

...The above paragraph is assuming recvmsg is able to deal with that and 
doesn't choke because of the changing copied_seq. I'd have to audit it 
once again to verify that it's really ok but I don't see any particular 
reason why it couldn't be possible to make recvmsg to not care on the 
tcp_check_urg side copied_seq changes but I'd like to hear a clear 
confirmation on that from you too.

> I also buy the argument that perhaps we should get rid of the log message, 

I'm not against the log message here. It's still useful for detecting real 
userspace races but the urg vs. MSG_PEEK case is what I'm not so sure is 
as evil in itself as claimed. I'm not sure if you meant the latter?

> but look at how it helped us find this kernel regression :-)

Heh, the regression was quite devastating yeah... ...limited to that check
itself :-). However, I had to audit rest of the loop too to check for 
other similar problems, so it certainly had more potential that just 
spurious printout (luckily I didn't find any other problem like this). 

-- 
 i.

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 19 May 2009 07:33:02 +0300 (EEST)

> Sure, the copied_seq is moving, but what I'm after is that does it really 
> make any difference from tcp_recvmsg point of view? It certainly triggers 
> the message but that won't work as a proof of the evilness for me.
> 
> ...The above paragraph is assuming recvmsg is able to deal with that and 
> doesn't choke because of the changing copied_seq. I'd have to audit it 
> once again to verify that it's really ok but I don't see any particular 
> reason why it couldn't be possible to make recvmsg to not care on the 
> tcp_check_urg side copied_seq changes but I'd like to hear a clear 
> confirmation on that from you too.

recvmsg can deal with it fine, because we reset the peek_seq when we
print out that message.

There is no way that an application writer has any clue about this
interaction, where peeked bytes disappear and then reappear in
the out-of-band URG byte.  That's why the message is there.

Re: [PATCH v2] tcp: fix MSG_PEEK race check

[Matthias Andree]

David Miller schrieb:

> Since I now understand your urg_hole code I'm going to apply your v2
> patch which takes care of the URG stuff.  I also buy the argument that
> perhaps we should get rid of the log message, but look at how it
> helped us find this kernel regression :-)

Hi,

...and this discovery is a reason to leave it in, and perhaps make sure
that the check code is properly linked (through comments for lack of better
means) to the actual data transfer functions.

Cheers
MA