Re: [PATCH] btrfs: remove redundant assignment and check on variable ret
On 09/05/17 17:55, Liu Bo wrote:
> On Sat, May 06, 2017 at 11:01:05PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Variable ret is assigned to zero and is always zero throughout the
>> function. Thus the check for ret being less than zero is always
>> false and so mapping_set_error always has an -EIO error passed to
>> it. Hence we can remove the redundant assignment and check on ret.
>>
>> Detected by CoverityScan, CID#1414312 ("Logically dead code")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>> fs/btrfs/extent_io.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index d8da3edf2ac3..7922cd34ba82 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2447,7 +2447,6 @@ void end_extent_writepage(struct page *page, int err,
>> u64 start, u64 end)
>> {
>> int uptodate = (err == 0);
>> struct extent_io_tree *tree;
>> -int ret = 0;
>>
>> tree = _I(page->mapping->host)->io_tree;
>>
>> @@ -2458,8 +2457,7 @@ void end_extent_writepage(struct page *page, int err,
>> u64 start, u64 end)
>> if (!uptodate) {
>> ClearPageUptodate(page);
>> SetPageError(page);
>> -ret = ret < 0 ? ret : -EIO;
>> -mapping_set_error(page->mapping, ret);
>> +mapping_set_error(page->mapping, -EIO);
>
> The passed 'err' should be used as what ret did, ie.
> ret = err < 0 ? err : -EIO;
Ah, so the err passed into the function should be used instead of ret,
which makes more sense. Got it. I'll send a fix for that.
Colin
>
> Thanks,
>
> -liubo
>> }
>> }
>>
>> --
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: fix dereference on inode->i_sb before inode null check
On 16/12/16 15:03, Jeff Mahoney wrote:
> On 12/16/16 7:20 AM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> inode is being deferenced and then inode is checked to see if it
>> is null, implying we potentially could have a null pointer deference
>> on inode.
>>
>> Found with static analysis by CoverityScan, CID 1389472
>>
>> Fix this by dereferencing inode only after the inode null check.
>>
>> Fixes: 0b246afa62b0cf5 ("btrfs: root->fs_info cleanup, add fs_info
>> convenience variables")
>
> Hi Colin -
>
> Thanks for the review. The right fix here is to eliminate the tests for
> inode == NULL entirely. This is a callback for exportfs, which will
> itself crash if dentry->d_inode or parent->d_inode is NULL. Removing
> the tests would be consistent with other file systems.
Ah, thanks for pointing that out. New patch on it's way soon.
Colin
>
> -Jeff
>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>> fs/btrfs/export.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
>> index 340d907..b746d2b 100644
>> --- a/fs/btrfs/export.c
>> +++ b/fs/btrfs/export.c
>> @@ -223,7 +223,7 @@ static int btrfs_get_name(struct dentry *parent, char
>> *name,
>> {
>> struct inode *inode = d_inode(child);
>> struct inode *dir = d_inode(parent);
>> -struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>> +struct btrfs_fs_info *fs_info;
>> struct btrfs_path *path;
>> struct btrfs_root *root = BTRFS_I(dir)->root;
>> struct btrfs_inode_ref *iref;
>> @@ -241,6 +241,7 @@ static int btrfs_get_name(struct dentry *parent, char
>> *name,
>> if (!S_ISDIR(dir->i_mode))
>> return -EINVAL;
>>
>> +fs_info = btrfs_sb(inode->i_sb);
>> ino = btrfs_ino(inode);
>>
>> path = btrfs_alloc_path();
>>
>
>
signature.asc
Description: OpenPGP digital signature
Re: BZ#101951, Overlayfs on top of btrfs causes kernel oops + freeze
On 16/02/16 16:11, Filipe Manana wrote:
> On Tue, Feb 16, 2016 at 4:08 PM, Colin Ian King
> <colin.k...@canonical.com> wrote:
>> On 16/02/16 15:51, Filipe Manana wrote:
>>> On Tue, Feb 16, 2016 at 3:38 PM, Colin Ian King
>>> <colin.k...@canonical.com> wrote:
>>>> Hi there,
>>>>
>>>> bug: https://bugzilla.kernel.org/show_bug.cgi?id=101951 and also
>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1532145
>>>>
>>>> Commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 ("overlayfs: Make f_path
>>>> always point to the overlay and f_inode to the underlay") resulted in an
>>>> issue when using a combination of btrfs and overlayfs. This is
>>>> noticeable when doing a fsync() on a file in a chroot with overlayfs on
>>>> top of btrfs; we hit a kernel oops in btrfs_sync_file() on
>>>> atomic_inc(>log_batch) because root is NULL.
>>>>
>>>> I've debugged this further and found that in btrfs_sync_file():
>>>>
>>>> struct inode *inode = d_inode(dentry);
>>>>
>>>> does not return the inode I expected when using the stacked overlay fs,
>>>> where as:
>>>>
>>>> struct inode *inode = file_inode(file);
>>>>
>>>> does.
>>>
>>> See the discussion at
>>> https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg48131.html
>>>
>>> You can get along with file_inode() in btrfs_sync_file(), but not
>>> later the fsync code path where we traverse the hierarchy up using
>>> dentries.
>>> More details on that thread.
>>
>> Ah, good. So was there any resolution on a way forward for a fix?
>
> Nop.
>
OK, so chroots don't work, that's a bit of a show stopper :-/
>>
>>>
>>>>
>>>> However, I'm not well at all well versed in btrfs, so I am not confident
>>>> this is a actually correct. Any comments?
>>>>
>>>> Colin
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majord...@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BZ#101951, Overlayfs on top of btrfs causes kernel oops + freeze
On 16/02/16 15:51, Filipe Manana wrote:
> On Tue, Feb 16, 2016 at 3:38 PM, Colin Ian King
> <colin.k...@canonical.com> wrote:
>> Hi there,
>>
>> bug: https://bugzilla.kernel.org/show_bug.cgi?id=101951 and also
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1532145
>>
>> Commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 ("overlayfs: Make f_path
>> always point to the overlay and f_inode to the underlay") resulted in an
>> issue when using a combination of btrfs and overlayfs. This is
>> noticeable when doing a fsync() on a file in a chroot with overlayfs on
>> top of btrfs; we hit a kernel oops in btrfs_sync_file() on
>> atomic_inc(>log_batch) because root is NULL.
>>
>> I've debugged this further and found that in btrfs_sync_file():
>>
>> struct inode *inode = d_inode(dentry);
>>
>> does not return the inode I expected when using the stacked overlay fs,
>> where as:
>>
>> struct inode *inode = file_inode(file);
>>
>> does.
>
> See the discussion at
> https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg48131.html
>
> You can get along with file_inode() in btrfs_sync_file(), but not
> later the fsync code path where we traverse the hierarchy up using
> dentries.
> More details on that thread.
Ah, good. So was there any resolution on a way forward for a fix?
>
>>
>> However, I'm not well at all well versed in btrfs, so I am not confident
>> this is a actually correct. Any comments?
>>
>> Colin
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
BZ#101951, Overlayfs on top of btrfs causes kernel oops + freeze
Hi there,
bug: https://bugzilla.kernel.org/show_bug.cgi?id=101951 and also
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1532145
Commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 ("overlayfs: Make f_path
always point to the overlay and f_inode to the underlay") resulted in an
issue when using a combination of btrfs and overlayfs. This is
noticeable when doing a fsync() on a file in a chroot with overlayfs on
top of btrfs; we hit a kernel oops in btrfs_sync_file() on
atomic_inc(>log_batch) because root is NULL.
I've debugged this further and found that in btrfs_sync_file():
struct inode *inode = d_inode(dentry);
does not return the inode I expected when using the stacked overlay fs,
where as:
struct inode *inode = file_inode(file);
does.
However, I'm not well at all well versed in btrfs, so I am not confident
this is a actually correct. Any comments?
Colin
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
