[PATCH 1/3] fs: befs: Lookup must return error code only on real error

[PATCH 1/3] fs: befs: Lookup must return error code only on real error

[Salah Triki]

File not found is not an error and lookup must return error code only
on real error, otherwise creating inodes with functions like create,
mkdir and so on will fail.

Signed-off-by: Salah Triki <salah.triki@acm.org>
---
 fs/befs/linuxvfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index c734f21..e0bd6c7 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -184,7 +184,7 @@ befs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 
 	if (ret == BEFS_BT_NOT_FOUND) {
 		befs_debug(sb, "<--- %s %pd not found", __func__, dentry);
-		return ERR_PTR(-ENOENT);
+		return NULL;
 
 	} else if (ret != BEFS_OK || offset == 0) {
 		befs_warning(sb, "<--- %s Error", __func__);
-- 
1.9.1

[PATCH 2/3] fs: befs: Insert NULL inode to dentry

[Salah Triki]

As VFS expects, lookup inserts NULL inode to dentry when the named
inode does not exist.

Signed-off-by: Salah Triki <salah.triki@acm.org>
---
 fs/befs/linuxvfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index e0bd6c7..91740dd 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -184,6 +184,7 @@ befs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 
 	if (ret == BEFS_BT_NOT_FOUND) {
 		befs_debug(sb, "<--- %s %pd not found", __func__, dentry);
+		d_add(dentry, NULL);
 		return NULL;
 
 	} else if (ret != BEFS_OK || offset == 0) {
-- 
1.9.1

[PATCH 3/3] fs: befs: Increment i_count when inode is found

[Salah Triki]

As VFS expects, i_count field is incremented when the named inode is found.

Signed-off-by: Salah Triki <salah.triki@acm.org>
---
 fs/befs/linuxvfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index 91740dd..05153b3 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -198,6 +198,8 @@ befs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 
 	d_add(dentry, inode);
 
+	inode->i_count++;
+
 	befs_debug(sb, "<--- %s", __func__);
 
 	return NULL;
-- 
1.9.1

Re: [PATCH 3/3] fs: befs: Increment i_count when inode is found

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Hi,

[auto build test ERROR on v4.7-rc1]
[also build test ERROR on next-20160603]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Salah-Triki/fs-befs-Lookup-must-return-error-code-only-on-real-error/20160605-025608
config: x86_64-randconfig-x013-201623 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   fs/befs/linuxvfs.c: In function 'befs_lookup':
>> fs/befs/linuxvfs.c:201:16: error: wrong type argument to increment
     inode->i_count++;
                   ^~

vim +201 fs/befs/linuxvfs.c

   195		inode = befs_iget(dir->i_sb, (ino_t) offset);
   196		if (IS_ERR(inode))
   197			return ERR_CAST(inode);
   198	
   199		d_add(dentry, inode);
   200	
 > 201		inode->i_count++;
   202	
   203		befs_debug(sb, "<--- %s", __func__);
   204	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25107 bytes --]

Re: [PATCH 3/3] fs: befs: Increment i_count when inode is found

[Al Viro]

On Sat, Jun 04, 2016 at 07:53:21PM +0100, Salah Triki wrote:
> As VFS expects, i_count field is incremented when the named inode is found.

VFS expects no such thing.  Incidentally, you have neither bothered to
check other filesystems nor cared to look at fs/inode.c.  OK, suppose you
have found a huge multi-filesystem bug - such things happen from time to
time.  But you have not even bothered to test your conjecture; this "fix"
had never been tried or even compiled.

What actually happens is that iget_locked() acquires a reference to inode.
That reference is either dropped by iget_failed() (called by befs_iget()
in case of failure to read and initialized the sucker) or used up by
d_add() as a reference to hold dentry->d_inode.

Similar situation holds for other filesystems; they do not need to manipulate
i_count at all.

Re: [PATCH 1/3] fs: befs: Lookup must return error code only on real error

[Al Viro]

On Sat, Jun 04, 2016 at 07:53:19PM +0100, Salah Triki wrote:
> File not found is not an error and lookup must return error code only
> on real error, otherwise creating inodes with functions like create,
> mkdir and so on will fail.

You do realize that befs is read-only and doesn't have ->mkdir() and friends,
right?  Patch is almost correct (you want d_add(dentry, NULL) if you go
that way, or you'll be calling ->lookup() ever time anyway), but it doesn't
make much sense on its own.

Re: [PATCH 3/3] fs: befs: Increment i_count when inode is found

[Salah Triki]

On Sat, Jun 04, 2016 at 08:32:42PM +0100, Al Viro wrote:
> On Sat, Jun 04, 2016 at 07:53:21PM +0100, Salah Triki wrote:
> > As VFS expects, i_count field is incremented when the named inode is found.
> 
> VFS expects no such thing.  Incidentally, you have neither bothered to
> check other filesystems nor cared to look at fs/inode.c.  OK, suppose you
> have found a huge multi-filesystem bug - such things happen from time to
> time.  But you have not even bothered to test your conjecture; this "fix"
> had never been tried or even compiled.
> 
> What actually happens is that iget_locked() acquires a reference to inode.
> That reference is either dropped by iget_failed() (called by befs_iget()
> in case of failure to read and initialized the sucker) or used up by
> d_add() as a reference to hold dentry->d_inode.
> 
> Similar situation holds for other filesystems; they do not need to manipulate
> i_count at all.

thanks for your comments