Hi Andrew and everybody else,
I looked through the pre-git history and seem to have found the reason of how
the current code had come to be, and why Sergei's fix seems to involve
re-enable "#if 0"'ed out code. It is this - I think the code was wrong on entry,
in that the first !REF_PAGES should be REF_PAGES. Then the "remove old
debug code"
commit below did not remove the right stuff.
Looking at the old code, I think REF_PAGES may have started out being
1 (i.e. pages are released right after create, then get/put when needed, no
need to release on bnode_free),
then the code was modified for efficiency so that pages are not released
on bnode_create and not put/get at bnode_put/get but release at bnode_free.
But it was still largely working in the REF_PAGE=1 mode (and with the commented
out release at bnode_free, which is supposed to be spanned by a !REF_PAGE).
Then it was flipped over, and seems to work, and things were forgotten.
I think the release at bnode_free was commented out because the person who
wrote it wasn't sure - I am not sure either, since it seems like there might be
other/more places that one might need to release the pages than just
at bnode_free().
Does this train of thought make sense?
Hin-Tak
<commit>
commit a5e3985fa014029eb6795664c704953720cc7f7d
Author: Roman Zippel <zippel@xxxxxxxxxxxxxx>
Date: Tue Sep 6 15:18:47 2005 -0700
[PATCH] hfs: remove debug code
This removes some old debug code, which is no longer needed.
Signed-off-by: Roman Zippel <zippel@xxxxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx>
diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index a096c5a..3d5cdc6 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -13,8 +13,6 @@
#include "btree.h"
-#define REF_PAGES 0
-
void hfs_bnode_read(struct hfs_bnode *node, void *buf,
int off, int len)
{
@@ -289,9 +287,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct
hfs_btree *tree, u32 cnid)
page_cache_release(page);
goto fail;
}
-#if !REF_PAGES
page_cache_release(page);
-#endif
node->page[i] = page;
}
@@ -449,13 +445,6 @@ void hfs_bnode_get(struct hfs_bnode *node)
{
if (node) {
atomic_inc(&node->refcnt);
-#if REF_PAGES
- {
- int i;
- for (i = 0; i < node->tree->pages_per_bnode; i++)
- get_page(node->page[i]);
- }
-#endif
dprint(DBG_BNODE_REFS, "get_node(%d:%d): %d\n",
node->tree->cnid, node->this, atomic_read(&node->refcnt));
}
@@ -472,20 +461,12 @@ void hfs_bnode_put(struct hfs_bnode *node)
node->tree->cnid, node->this, atomic_read(&node->refcnt));
if (!atomic_read(&node->refcnt))
BUG();
- if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) {
-#if REF_PAGES
- for (i = 0; i < tree->pages_per_bnode; i++)
- put_page(node->page[i]);
-#endif
+ if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock))
return;
- }
for (i = 0; i < tree->pages_per_bnode; i++) {
if (!node->page[i])
continue;
mark_page_accessed(node->page[i]);
-#if REF_PAGES
- put_page(node->page[i]);
-#endif
}
if (test_bit(HFS_BNODE_DELETED, &node->flags)) {
diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 8868d3b..b85abc6 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -18,8 +18,6 @@
#include "hfsplus_fs.h"
#include "hfsplus_raw.h"
-#define REF_PAGES 0
-
/* Copy a specified range of bytes from the raw data of a node */
void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
{
@@ -450,9 +448,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct
hfs_btree *tree, u32 cnid)
page_cache_release(page);
goto fail;
}
-#if !REF_PAGES
page_cache_release(page);
-#endif
node->page[i] = page;
}
@@ -612,13 +608,6 @@ void hfs_bnode_get(struct hfs_bnode *node)
{
if (node) {
atomic_inc(&node->refcnt);
-#if REF_PAGES
- {
- int i;
- for (i = 0; i < node->tree->pages_per_bnode; i++)
- get_page(node->page[i]);
- }
-#endif
dprint(DBG_BNODE_REFS, "get_node(%d:%d): %d\n",
node->tree->cnid, node->this, atomic_read(&node->refcnt));
}
@@ -635,20 +624,12 @@ void hfs_bnode_put(struct hfs_bnode *node)
node->tree->cnid, node->this, atomic_read(&node->refcnt));
if (!atomic_read(&node->refcnt))
BUG();
- if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) {
-#if REF_PAGES
- for (i = 0; i < tree->pages_per_bnode; i++)
- put_page(node->page[i]);
-#endif
+ if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock))
return;
- }
for (i = 0; i < tree->pages_per_bnode; i++) {
if (!node->page[i])
continue;
mark_page_accessed(node->page[i]);
-#if REF_PAGES
- put_page(node->page[i]);
-#endif
}
if (test_bit(HFS_BNODE_DELETED, &node->flags)) {
</commit>
On 18 June 2015 at 00:41, Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx> wrote:
> ------------------------------
> On Tue, Jun 9, 2015 7:06 PM BST Anton Altaparmakov wrote:
>
>>Hi Andrew,
>>
>>Can you please take this patch up and get it merged into mainline? Despite Vyacheslav's lamentations this patch is obviously correct. __hfs_bnode_create() is called in two places from the driver and the pages it returned are kmap()-ed and used (both read and write) so it is quite obvious they cannot be page_cache_release()-d until after they are kunmap()-ed instead of before they are even kmap()-ed as happens without this patch and this patch fixes exactly this by moving the page_cache_release() calls to after the pages are kunmap()-ed.
>>
>>Feel free to add:
>>
>>Reviewed-by: Anton Altaparmakov <anton@xxxxxxxxxx>
>>
>>Thanks a lot in advance!
>>
>>Best regards,
>>
>> Anton
>
> I went around and looked at the code and Anton is right - __hfs_bnode_create() is called from hfs_bnode_create() and bfs_bnode_find(), and the pages are played with afterwards a bit further, so page_cache_release() at where the current location is, is wrong.
>
> Since I have a way of seeing a likely-related problem, I'll try testing this change in the next few days and possibly adding a Signed-off at the point.
>
> BTW, there is a similiar problem with the hfs (no plus) driver also; the "if 0" part is commented out with // c++ style comments instead.
> I'm looking over the pre-git history to see how that comes about.
>
> Hin-Tak
>
>>> On 7 Jun 2015, at 03:42, Sergei Antonov <saproj@xxxxxxxxx> wrote:
>>>
>>> Fix this bugreport by Sasha Levin:
>>> http://lkml.org/lkml/2015/2/20/85 ("use after free")
>>> Make sure mapped pages are available for the entire lifetime of hfs_bnode.
>>>
>>> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>>> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
>>> Cc: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>>> Cc: Sougata Santra <sougata@xxxxxxxxxx>
>>> Reported-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
>>> Signed-off-by: Sergei Antonov <saproj@xxxxxxxxx>
>>> ---
>>> fs/hfsplus/bnode.c | 6 ++----
>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
>>> index 759708f..5af50fb 100644
>>> --- a/fs/hfsplus/bnode.c
>>> +++ b/fs/hfsplus/bnode.c
>>> @@ -454,7 +454,6 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
>>> page_cache_release(page);
>>> goto fail;
>>> }
>>> - page_cache_release(page);
>>> node->page[i] = page;
>>> }
>>>
>>> @@ -566,13 +565,12 @@ node_error:
>>>
>>> void hfs_bnode_free(struct hfs_bnode *node)
>>> {
>>> -#if 0
>>> int i;
>>>
>>> - for (i = 0; i < node->tree->pages_per_bnode; i++)
>>> + for (i = 0; i < node->tree->pages_per_bnode; i++) {
>>> if (node->page[i])
>>> page_cache_release(node->page[i]);
>>> -#endif
>>> + }
>>> kfree(node);
>>> }
>>>
>>> --
>>> 2.3.0
>>>
>>> --
>>> 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
>>
>>--
>>Anton Altaparmakov <anton at tuxera.com> (replace at with @)
>>Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
>>Linux NTFS maintainer
>>
>
--
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
