From: Jérôme Glisse jglisse@redhat.com
(Result of code inspection only, i do not have a bug, nor know a bug that would be explain by this issue. Is there a kernel trace database one can query for that ?)
When fs is mounted with nobh temporary buffer_head are created during write and they are only associated with the page when a filesystem error happen. When this happen nobh_write_begin() or nobh_write_end() call attach_nobh_buffers() which expect that provided buffer_head list is circular (ie tail entry point to head entry). If it is not the case it will dereference the last pointer in the list which is NULL (last item in the list point to NULL see alloc_page_buffers()) and thus SEGFAULT.
Hence nobh_write_begin() must make the buffer_head list circular as alloc_page_buffers() is not responsible for doing that.
(This might worth including in 4.14 as i don't think it can regress anything but i am not a filesystem expert).
Note i did not make the list circular inside attach_nobh_buffers() as some patchset i am working on also expect the list to be circular no matter what. But if people are more confortable with me doing that in my patchset the fix can be move to attach_nobh_buffers().
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Al Viro viro@zeniv.linux.org.uk Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Cc: Linus Torvalds torvalds@linux-foundation.org --- fs/buffer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/buffer.c b/fs/buffer.c index 170df856bdb9..6bc47c11d6ac 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -2598,7 +2598,7 @@ int nobh_write_begin(struct address_space *mapping, struct inode *inode = mapping->host; const unsigned blkbits = inode->i_blkbits; const unsigned blocksize = 1 << blkbits; - struct buffer_head *head, *bh; + struct buffer_head *head, *bh, *tail; struct page *page; pgoff_t index; unsigned from, to; @@ -2643,6 +2643,13 @@ int nobh_write_begin(struct address_space *mapping, ret = -ENOMEM; goto out_release; } + /* We need to make buffer_head list circular to avoid NULL SEGFAULT */ + bh = head; + do { + tail = bh; + bh = bh->b_this_page; + } while (bh); + tail->b_this_page = head;
block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
On Thu, Nov 9, 2017 at 1:49 PM, jglisse@redhat.com wrote:
(Result of code inspection only, i do not have a bug, nor know a bug that would be explain by this issue. Is there a kernel trace database one can query for that ?)
This is intentional.
See the comment above the code you added:
* Be careful: the buffer linked list is a NULL terminated one, rather * than the circular one we're used to.
and then nobh_write_end() does:
struct buffer_head *head = fsdata; ... while (head) { bh = head; head = head->b_this_page; free_buffer_head(bh); }
so it *depends* on the bh list being NULL-terminated.
So your patch is definitely wrong and breaks that nobh_write_end() case.
Which is not to say that there couldn't be a NULL pointer dereference in some error path exactly because this code intentionally breaks the normal rules.
But no, I'm definitely not applying this patch as-is, and not just before 4.14.
Linus
On Fri, Nov 10, 2017 at 12:38:29PM -0800, Linus Torvalds wrote:
On Thu, Nov 9, 2017 at 1:49 PM, jglisse@redhat.com wrote:
(Result of code inspection only, i do not have a bug, nor know a bug that would be explain by this issue. Is there a kernel trace database one can query for that ?)
This is intentional.
See the comment above the code you added:
* Be careful: the buffer linked list is a NULL terminated one, rather * than the circular one we're used to.
and then nobh_write_end() does:
struct buffer_head *head = fsdata;
... while (head) { bh = head; head = head->b_this_page; free_buffer_head(bh); }
so it *depends* on the bh list being NULL-terminated.
So your patch is definitely wrong and breaks that nobh_write_end() case.
Which is not to say that there couldn't be a NULL pointer dereference in some error path exactly because this code intentionally breaks the normal rules.
But no, I'm definitely not applying this patch as-is, and not just before 4.14.
You are right, i will rework that as part of my patchset, this is all 4.16 material at best anyway. If i get sometime i will try to trigger the issue on nobh this week and send a fix that only make the list circular inside attach_nobh_buffers() which is were the SEGFAULT would happen and where we would need to make it circular before attaching to the page as other code in the kernel expect that list to be circular.
Jérôme
linux-stable-mirror@lists.linaro.org