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

["Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>]

[-- 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.