On Mon, 8 Jun 2015 18:50:00 +0200 Sergei Antonov <saproj@xxxxxxxxx> wrote: > >> You are basically saying you don___t understand it. Too bad, because the > >> bug is very simple. It is the ___use after free___ type of bug, and it can > >> be illustrated by this: > >> (1) void *ptr = malloc(___); > >> (2) free(ptr); > >> (3) memcpy(___, ptr, 1); > >> Guess which two of these three lines are executed in wrong order. > >> > >> My patch is about the same type of bug, but with memory pages mapping. > >> The driver currently accesses pages that may be unavailable, or > >> contain different data. The problem is more likely to occur when > >> memory is a limited resource. I reproduced it while running a > >> memory-hungry program. > > > > I worried not about myself but about potential readers of description of > > the fix. The description is completely obscure. And it needs to describe > > the fix in clear and descriptive manner. This is my request. Please, > > describe the fix in a clear way. > > The description is just right. Yes, I too would like to hear much more about your thinking on this, and a detailed description of the bug and how the patch fixes it. The code is distressingly undocumented and has been that way since Roman Zippel's original commit in 2004. >From the looks of it, that loop in __hfs_bnode_create() is simply doing readahead and is designed as a performance optimisation. The pages are pulled into pagecache in the expectation that they will soon be accessed. What your patch does is to instead pin the pages in pagecache until the bnode is freed. If we're going to do this then we need to be very careful about worst-case scenarios: we could even run the machine out of memory. If I'm correct, and this is just readahead then the bug lies elsewhere: if other parts of hfsplus are assuming that this memory is in pagecache then that's an error - that code (where is it?) should instead be performing a pagecache lookup and if the page isn't present, read it from disk again. But for others to be able to review and understand this change and suggest alternatives, we'll need a much much better changelog! -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
