[RFC PATCH] fs: introduce check for drop/inc_nlink

[RFC PATCH] fs: introduce check for drop/inc_nlink

[cheng.lin130]

[-- Attachment #1.1.1: Type: text/plain, Size: 847 bytes --]

From: Cheng Lin <cheng.lin130@zte.com.cn>

Avoid inode nlink overflow or underflow.

Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
---
 fs/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 67611a360..8e6d62dc4 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -328,6 +328,9 @@ static void destroy_inode(struct inode *inode)
 void drop_nlink(struct inode *inode)
 {
 	WARN_ON(inode->i_nlink == 0);
+	if (unlikely(inode->i_nlink == 0))
+		return;
+
 	inode->__i_nlink--;
 	if (!inode->i_nlink)
 		atomic_long_inc(&inode->i_sb->s_remove_count);
@@ -388,6 +391,9 @@ void inc_nlink(struct inode *inode)
 		atomic_long_dec(&inode->i_sb->s_remove_count);
 	}
 
+	if (unlikely(inode->i_nlink == ~0U))
+		return;
+
 	inode->__i_nlink++;
 }
 EXPORT_SYMBOL(inc_nlink);
-- 
2.18.1

[-- Attachment #1.1.2: Type: text/html , Size: 1907 bytes --]

Re: [RFC PATCH] fs: introduce check for drop/inc_nlink

[Christian Brauner]

On Fri, Oct 13, 2023 at 03:27:30PM +0800, cheng.lin130@zte.com.cn wrote:
> From: Cheng Lin <cheng.lin130@zte.com.cn>
> 
> Avoid inode nlink overflow or underflow.
> 
> Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> ---

I'm very confused. There's no explanation why that's needed. As it
stands it's not possible to provide a useful review.

I'm not saying it's wrong. I just don't understand why and even if this
should please show up in the commit message.

>  fs/inode.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 67611a360..8e6d62dc4 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -328,6 +328,9 @@ static void destroy_inode(struct inode *inode)
>  void drop_nlink(struct inode *inode)
>  {
>  	WARN_ON(inode->i_nlink == 0);
> +	if (unlikely(inode->i_nlink == 0))
> +		return;
> +
>  	inode->__i_nlink--;
>  	if (!inode->i_nlink)
>  		atomic_long_inc(&inode->i_sb->s_remove_count);
> @@ -388,6 +391,9 @@ void inc_nlink(struct inode *inode)
>  		atomic_long_dec(&inode->i_sb->s_remove_count);
>  	}
>  
> +	if (unlikely(inode->i_nlink == ~0U))
> +		return;
> +
>  	inode->__i_nlink++;
>  }
>  EXPORT_SYMBOL(inc_nlink);
> -- 
> 2.18.1

Re: [RFC PATCH] fs: introduce check for drop/inc_nlink

[cheng.lin130]

> On Fri, Oct 13, 2023 at 03:27:30PM +0800, cheng.lin130@zte.com.cn wrote:
> > From: Cheng Lin <cheng.lin130@zte.com.cn>
> >
> > Avoid inode nlink overflow or underflow.
> >
> > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > ---
> I'm very confused. There's no explanation why that's needed. As it
> stands it's not possible to provide a useful review.
> I'm not saying it's wrong. I just don't understand why and even if this
> should please show up in the commit message.
In an xfs issue, there was an nlink underflow of a directory inode. There
is a key information in the kernel messages, that is the WARN_ON from
drop_nlink(). However, VFS did not prevent the underflow. I'm not sure
if this behavior is inadvertent or specifically designed. As an abnormal
situation, perhaps prohibiting nlink overflow or underflow is a better way
to handle it.
Request for your comment.

Re: [RFC PATCH] fs: introduce check for drop/inc_nlink

[Darrick J. Wong]

On Fri, Oct 13, 2023 at 05:40:57PM +0800, cheng.lin130@zte.com.cn wrote:
> > On Fri, Oct 13, 2023 at 03:27:30PM +0800, cheng.lin130@zte.com.cn wrote:
> > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > >
> > > Avoid inode nlink overflow or underflow.
> > >
> > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > ---
> > I'm very confused. There's no explanation why that's needed. As it
> > stands it's not possible to provide a useful review.
> > I'm not saying it's wrong. I just don't understand why and even if this
> > should please show up in the commit message.
> In an xfs issue, there was an nlink underflow of a directory inode. There
> is a key information in the kernel messages, that is the WARN_ON from
> drop_nlink(). However, VFS did not prevent the underflow. I'm not sure
> if this behavior is inadvertent or specifically designed. As an abnormal
> situation, perhaps prohibiting nlink overflow or underflow is a better way
> to handle it.
> Request for your comment.

I was trying to steer you towards modifying vfs_rmdir and vfs_unlink to
check that i_nlink of the files involved aren't somehow already zero
and returning -EFSCORRUPTED if they are.  Not messing around with
drop_nlink.

--D

Re: [RFC PATCH] fs: introduce check for drop/inc_nlink

[cheng.lin130]

> On Fri, Oct 13, 2023 at 05:40:57PM +0800, cheng.lin130@zte.com.cn wrote:
> > > On Fri, Oct 13, 2023 at 03:27:30PM +0800, cheng.lin130@zte.com.cn wrote:
> > > > From: Cheng Lin <cheng.lin130@zte.com.cn>
> > > >
> > > > Avoid inode nlink overflow or underflow.
> > > >
> > > > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > > > ---
> > > I'm very confused. There's no explanation why that's needed. As it
> > > stands it's not possible to provide a useful review.
> > > I'm not saying it's wrong. I just don't understand why and even if this
> > > should please show up in the commit message.
> > In an xfs issue, there was an nlink underflow of a directory inode. There
> > is a key information in the kernel messages, that is the WARN_ON from
> > drop_nlink(). However, VFS did not prevent the underflow. I'm not sure
> > if this behavior is inadvertent or specifically designed. As an abnormal
> > situation, perhaps prohibiting nlink overflow or underflow is a better way
> > to handle it.
> > Request for your comment.
> I was trying to steer you towards modifying vfs_rmdir and vfs_unlink to
> check that i_nlink of the files involved aren't somehow already zero
> and returning -EFSCORRUPTED if they are.  Not messing around with
> drop_nlink.
> --D
It seems that VFS is not very concerned about the values of filesystem,
such as inode nlinks. And the defination of whether it is correct or incorrect
is left to the specific filesystem. However, in some places do have limit
up to s_max_links. Perhaps the limit down to zero is also reasonable.
Is that right？