| Message ID | Pine.LNX.4.64.0905070931590.12068@wrl-59.cs.helsinki.fi |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));