Data Corruption bug with Samba's vfs_iouring and Linux 5.6.7/5.7rc3
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 833 bytes
Desc: OpenPGP digital signature
More information about the samba-technical