Data Corruption bug with Samba's vfs_iouring and Linux 5.6.7/5.7rc3

Stefan Metzmacher metze at samba.org
Fri May 8 22:21:32 UTC 2020


Am 09.05.20 um 00:03 schrieb Jeremy Allison:
> On Fri, May 08, 2020 at 02:50:55PM -0700, Jeremy Allison wrote:
>> On Fri, May 08, 2020 at 11:40:30PM +0200, Stefan Metzmacher wrote:
>>> Am 08.05.20 um 22:51 schrieb Jeremy Allison:
>>>> On Fri, May 08, 2020 at 01:47:09PM -0700, Jeremy Allison via samba-technical wrote:
>>>>> On Fri, May 08, 2020 at 09:35:31PM +0200, Stefan Metzmacher via samba-technical wrote:
>>>>>>
>>>>>> Thanks very much Jeremy! I didn't noticed that.
>>>>>>
>>>>>> I guess the attached patch should be able to fix the recursion.
>>>>>
>>>>> Oh Metze that's *really* ugly :-). I thought about
>>>>> doing that in my code and decided it was in too bad
>>>>> taste to live :-).
>>>>>
>>>>> Is there a cleaner way than putting "busy" and "retry"
>>>>> variables in the config struct ?
>>>>
>>>> And a "Goto again" as well :-(. Bleegh.
>>>
>>> This version would actually work and looks a bit similar to
>>> your version.
>>>
>>> Can you life with that version?
>>
>> Yes I can live with that :-). It at least moves the horror
>> to the wrapper function where you can at least concentrate
>> all your attention as to why it's doing what it's doing :-).
>>
>> RB+ from me if you add a comment header above the function
>> as well as in the commit so it explains what it's doing.
>>
>> Feel free to crib my ascii art to explain in the header
>> comment too :-).
> 
> There's just one final comment I want to make before
> my version goes quietly into that good night... :-).
> 
> I will grant that your version leaves a cleaner set of code
> paths throughout the io_uring pread/pwrite code and deals with
> the short read/write issues in a more "natural" way
> without my extra logic branches dealing with "short read"
> return etc.
> 
> But the fact that you missed the recursion might make
> you consider if indeed it's a simpler fix than having
> all the extra logic made explicit. Sometimes explicit
> is good to draw people's attention to complexity.
> 
> Having said that, I'll go with whatever you decide
> on this one :-). I already gave me RB+ for the fix,
> so I'll support whichever one you pick. But it would
> be good to get the fix pushed and back-ported into
> 4.12 asap to cover the data corruption.

I think we need to check the write case.

Do you know what happens when the kernel gets a negative offset?

Is there a way to do an O_APPEND?
Doing an fstat to get EOF is racy.
As well as split one write into multiple calls.

But maybe it's ok as a start.

Once we're done I'll ask Karolin to create a new 4.12 release.

> Thanks for working on this with me. I do learn things
> from the elegance of your code (usually :-).

Thanks for being so patient!
Your work was really useful in order to make it easier for
me to find my solution.

metze


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20200509/d464efc4/signature.sig>


More information about the samba-technical mailing list