* [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
@ 2011-06-28 3:34 Tsutomu Itoh
2011-06-28 14:22 ` Josef Bacik
0 siblings, 1 reply; 6+ messages in thread
From: Tsutomu Itoh @ 2011-06-28 3:34 UTC (permalink / raw)
To: linux-btrfs; +Cc: chris.mason
The return value of btrfs_lookup_dentry is checked so that
the panic such as illegal address reference should not occur.
Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
fs/btrfs/inode.c | 1 +
fs/btrfs/ioctl.c | 10 +++++++++-
2 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 447612d..1364ae8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4082,6 +4082,7 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
inode = btrfs_lookup_dentry(dir, dentry);
if (IS_ERR(inode))
return ERR_CAST(inode);
+ BUG_ON(!inode);
return d_splice_alias(inode, dentry);
}
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a3c4751..39c62d3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -325,6 +325,7 @@ static noinline int create_subvol(struct btrfs_root *root,
struct btrfs_root *new_root;
struct dentry *parent = dget_parent(dentry);
struct inode *dir;
+ struct inode *inode;
int ret;
int err;
u64 objectid;
@@ -437,7 +438,14 @@ static noinline int create_subvol(struct btrfs_root *root,
BUG_ON(ret);
- d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry));
+ inode = btrfs_lookup_dentry(dir, dentry);
+ if (IS_ERR(inode)) {
+ ret = PTR_ERR(inode);
+ goto fail;
+ }
+ BUG_ON(!inode);
+
+ d_instantiate(dentry, inode);
fail:
dput(parent);
if (async_transid) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
2011-06-28 3:34 [PATCH] Btrfs: fix error check of btrfs_lookup_dentry() Tsutomu Itoh
@ 2011-06-28 14:22 ` Josef Bacik
2011-06-29 0:15 ` Tsutomu Itoh
0 siblings, 1 reply; 6+ messages in thread
From: Josef Bacik @ 2011-06-28 14:22 UTC (permalink / raw)
To: Tsutomu Itoh; +Cc: linux-btrfs, chris.mason
On 06/27/2011 11:34 PM, Tsutomu Itoh wrote:
> The return value of btrfs_lookup_dentry is checked so that
> the panic such as illegal address reference should not occur.
>
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
Nack, please fix btrfs_lookup_dentry to return ERR_PTR(-ENOENT) if it
doesn't find something. Thanks,
Josef
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
2011-06-28 14:22 ` Josef Bacik
@ 2011-06-29 0:15 ` Tsutomu Itoh
0 siblings, 0 replies; 6+ messages in thread
From: Tsutomu Itoh @ 2011-06-29 0:15 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, chris.mason
(2011/06/28 23:22), Josef Bacik wrote:
> On 06/27/2011 11:34 PM, Tsutomu Itoh wrote:
>> The return value of btrfs_lookup_dentry is checked so that
>> the panic such as illegal address reference should not occur.
>>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>
> Nack, please fix btrfs_lookup_dentry to return ERR_PTR(-ENOENT) if it
> doesn't find something. Thanks,
OK, I will repost soon.
Thanks,
Tsutomu
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
@ 2013-12-13 0:51 Tsutomu Itoh
2013-12-16 15:27 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Tsutomu Itoh @ 2013-12-13 0:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: mfasheh
Clean up btrfs_lookup_dentry() to never return NULL, but PTR_ERR(-ENOENT)
instead. This keeps the return value convention consistent.
Callers who use btrfs_lookup_dentry() require a trivial update.
create_snapshot() in particular looks like it can also lose a BUG_ON(!inode)
which is not really needed - there seems less harm in returning ENOENT to
userspace at that point in the stack than there is to crash the machine.
Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
fs/btrfs/inode.c | 15 +++++++++++----
fs/btrfs/ioctl.c | 13 ++++++++++---
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3b4ffaf..f9b45a7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4910,7 +4910,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
return ERR_PTR(ret);
if (location.objectid == 0)
- return NULL;
+ return ERR_PTR(-ENOENT);
if (location.type == BTRFS_INODE_ITEM_KEY) {
inode = btrfs_iget(dir->i_sb, &location, root, NULL);
@@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry)
static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
unsigned int flags)
{
- struct dentry *ret;
+ struct inode *inode;
- ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
- return ret;
+ inode = btrfs_lookup_dentry(dir, dentry);
+ if (IS_ERR(inode)) {
+ if (PTR_ERR(inode) == -ENOENT)
+ inode = NULL;
+ else
+ return ERR_CAST(inode);
+ }
+
+ return d_splice_alias(inode, dentry);
}
unsigned char btrfs_filetype_table[] = {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9d46f60..716779c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -389,6 +389,7 @@ static noinline int create_subvol(struct inode *dir,
struct btrfs_root *new_root;
struct btrfs_block_rsv block_rsv;
struct timespec cur_time = CURRENT_TIME;
+ struct inode *inode;
int ret;
int err;
u64 objectid;
@@ -550,8 +551,14 @@ fail:
if (err && !ret)
ret = err;
- if (!ret)
- d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry));
+ if (!ret) {
+ inode = btrfs_lookup_dentry(dir, dentry);
+ if (IS_ERR(inode)) {
+ ret = PTR_ERR(inode);
+ goto out;
+ }
+ d_instantiate(dentry, inode);
+ }
out:
btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
return ret;
@@ -639,7 +646,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
ret = PTR_ERR(inode);
goto fail;
}
- BUG_ON(!inode);
+
d_instantiate(dentry, inode);
ret = 0;
fail:
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
2013-12-13 0:51 Tsutomu Itoh
@ 2013-12-16 15:27 ` David Sterba
2013-12-17 1:10 ` Tsutomu Itoh
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2013-12-16 15:27 UTC (permalink / raw)
To: Tsutomu Itoh; +Cc: linux-btrfs, mfasheh
On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh wrote:
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry)
> static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
> unsigned int flags)
> {
> - struct dentry *ret;
> + struct inode *inode;
>
> - ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
> - return ret;
> + inode = btrfs_lookup_dentry(dir, dentry);
> + if (IS_ERR(inode)) {
> + if (PTR_ERR(inode) == -ENOENT)
> + inode = NULL;
> + else
> + return ERR_CAST(inode);
> + }
> +
> + return d_splice_alias(inode, dentry);
btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...)
and the expanded back and forth with the DCACHE_NEED_LOOKUP flag.
a66e7cc626f42de6c745963fe0d807518fa49d39 added
39e3c9553f34381a1b664c27b0c696a266a5735e removed
d_splice_alias has been made robust in
a9049376ee05bf966bfe2b081b5071326856890a
"make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)"
you can drop the error handling from btrfs_lookup completely.
The rest looks ok.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()
2013-12-16 15:27 ` David Sterba
@ 2013-12-17 1:10 ` Tsutomu Itoh
0 siblings, 0 replies; 6+ messages in thread
From: Tsutomu Itoh @ 2013-12-17 1:10 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs
Hi, David,
On 2013/12/17 0:27, David Sterba wrote:
> On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh wrote:
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry)
>> static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
>> unsigned int flags)
>> {
>> - struct dentry *ret;
>> + struct inode *inode;
>>
>> - ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
>> - return ret;
>> + inode = btrfs_lookup_dentry(dir, dentry);
>> + if (IS_ERR(inode)) {
>> + if (PTR_ERR(inode) == -ENOENT)
>> + inode = NULL;
>> + else
>> + return ERR_CAST(inode);
>> + }
>> +
>> + return d_splice_alias(inode, dentry);
>
> btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...)
> and the expanded back and forth with the DCACHE_NEED_LOOKUP flag.
>
> a66e7cc626f42de6c745963fe0d807518fa49d39 added
> 39e3c9553f34381a1b664c27b0c696a266a5735e removed
>
> d_splice_alias has been made robust in
> a9049376ee05bf966bfe2b081b5071326856890a
> "make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)"
>
> you can drop the error handling from btrfs_lookup completely.
d_splice_alias() is called by the following formats if btrfs_lookup_dentry()
returns NULL before my patch is applied.
d_splice_alias(NULL, dentry);
However, d_splice_alias() is called by the following formats when
becoming the same situation after my patch is applied.
d_splice_alias(-ENOENT, dentry);
Therefore, I added the following error handling code.
+ if (IS_ERR(inode)) {
+ if (PTR_ERR(inode) == -ENOENT)
+ inode = NULL;
+ else
+ return ERR_CAST(inode);
+ }
Thanks,
Tsutomu
>
> The rest looks ok.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-17 1:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-28 3:34 [PATCH] Btrfs: fix error check of btrfs_lookup_dentry() Tsutomu Itoh
2011-06-28 14:22 ` Josef Bacik
2011-06-29 0:15 ` Tsutomu Itoh
-- strict thread matches above, loose matches on Subject: below --
2013-12-13 0:51 Tsutomu Itoh
2013-12-16 15:27 ` David Sterba
2013-12-17 1:10 ` Tsutomu Itoh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).