On 10/3/19 2:46 AM, Matthew Wilcox wrote:
> On Wed, Oct 02, 2019 at 08:19:59PM -0700, Eric Dumazet wrote:
>> Apparently a refactoring patch brought a bug, that was caught
>> by syzbot [1]
>
> That wasn't refactoring. As you know (because we talked about it at
> LSFMM), this is an enabling patch for supporting hch's work to fix
> get_user_pages().
Changing frags->size by skb_frag_size(frags) is what I call refactoring.
This was claimed in the changelog of your patch :
<quote>
net: Use skb accessors in network core
In preparation for unifying the skb_frag and bio_vec, use the fine
accessors which already exist and use skb_frag_t instead of
struct skb_frag_struct.
</quote>
I guess David trusted you enough and did not checked that you made other
changes.
>
>> Original code was correct, do not try to be smarter than the
>> compiler :/
>
> That wasn't an attempt to be smarter than the compiler. I was trying
> to keep the line length below 80 columns. Which you probably now see
> that you haven't done.
Seriously we do not care of the 80 column rule in this particular function,
we care about code correctness first.
We do not mix fixes and cleanups in the same patch.
>
> I must have a blind spot here. I can't see the difference between the
> two versions.
No worries.
The major difference is that with your code, when @remaining reaches zero,
it fetches an extra "size = skb_frag_size(frags);" at line 1807
as the syzbot report correctly points.
MAX_SKB_FRAGS is 17 on x86, meaning struct skb_shared_info frags[] array
has 17 elements.
Accessing the 18th element can trigger a page fault, if struct skb_shared_info
sits exactly at the end of a page.
while (remaining && (size != PAGE_SIZE ||
skb_frag_off(frags))) {
remaining -= size; // when this reaches 0
frags++; // this points to the next fragment in the array, not
initialized.
size = skb_frag_size(frags); // Crash here if crossing a page and next
page is unmapped.
}
Original code from Soheil was correct :
while (remaining && (frags->size != PAGE_SIZE ||
frags->page_offset)) {
remaining -= frags->size;
frags++;
}
My patch simply restore this code, keeping the accessors changes which were
perfectly fine,
thank you.
