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

[David Miller <davem@davemloft.net>]

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 :-)