[PATCH] inode_has_no_xattr() does not use proper sync

[PATCH] inode_has_no_xattr() does not use proper sync

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 933 bytes --]


inode.i_flags is modified without any proper
synchronisation used. inode_set_flags() is now used.

Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
Spinczyk)

Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 include/linux/fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c95c0807471f..54f3a21668a6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3446,7 +3446,7 @@ static inline int check_sticky(struct inode *dir,
struct inode *inode)
 static inline void inode_has_no_xattr(struct inode *inode)
 {
 	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & SB_NOSEC))
-		inode->i_flags |= S_NOSEC;
+		inode_set_flags(inode, S_NOSEC, S_NOSEC);
 }

 static inline bool is_root_inode(struct inode *inode)
-- 
2.19.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] inode_has_no_xattr() does not use proper sync

[Jan Kara]

On Tue 27-11-18 15:54:28, Alexander Lochmann wrote:
> 
> inode.i_flags is modified without any proper
> synchronisation used. inode_set_flags() is now used.
> 
> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
> Spinczyk)
> 
> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

Thanks for the patch! Couple notes to this patch:

1) This is a generic VFS helper as such, linux-fsdevel mailing list and VFS
maintainer Al Viro is the right forum to post this patch to. We do have
scripts/get_maintainer.pl script you can use on a patch / file to get idea
who's the best to post the change to. It is not perfect but usually works
fine.

2) It would be good to include stacktrace showing where the unlocked access
happens in the changelog. It is non-trivial to find it by brief inspection
as all standard filesystems call inode_has_no_xattr() under i_rwsem. This
problem is really specific to blkdev_write_iter() AFAICT.

3) Also can you please add comment into inode_has_no_xattr() like:
	/*
	 * blkdev_write_iter() can call this without i_rwsem, need to be
	 * careful with i_flags update.
	 */

								Honza
> ---
>  include/linux/fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c95c0807471f..54f3a21668a6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3446,7 +3446,7 @@ static inline int check_sticky(struct inode *dir,
> struct inode *inode)
>  static inline void inode_has_no_xattr(struct inode *inode)
>  {
>  	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & SB_NOSEC))
> -		inode->i_flags |= S_NOSEC;
> +		inode_set_flags(inode, S_NOSEC, S_NOSEC);
>  }
> 
>  static inline bool is_root_inode(struct inode *inode)
> -- 
> 2.19.1
> 



-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] inode_has_no_xattr() does not use proper sync

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 2388 bytes --]



Am 05.12.18 um 10:01 schrieb Jan Kara:
> On Tue 27-11-18 15:54:28, Alexander Lochmann wrote:
>>
>> inode.i_flags is modified without any proper
>> synchronisation used. inode_set_flags() is now used.
>>
>> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
>> Spinczyk)
>>
>> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
>> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
> 
> Thanks for the patch! Couple notes to this patch:
> 
> 1) This is a generic VFS helper as such, linux-fsdevel mailing list and VFS
> maintainer Al Viro is the right forum to post this patch to. We do have
> scripts/get_maintainer.pl script you can use on a patch / file to get idea
> who's the best to post the change to. It is not perfect but usually works
> fine.
Oh, that's my fault. I thought this ml was the right place.
> 
> 2) It would be good to include stacktrace showing where the unlocked access
> happens in the changelog. It is non-trivial to find it by brief inspection
> as all standard filesystems call inode_has_no_xattr() under i_rwsem. This
> problem is really specific to blkdev_write_iter() AFAICT.
> 
> 3) Also can you please add comment into inode_has_no_xattr() like:
> 	/*
> 	 * blkdev_write_iter() can call this without i_rwsem, need to be
> 	 * careful with i_flags update.
> 	 */
2) + 3) Done. Will post the patch asap.

- Alex
> 
> 								Honza
>> ---
>>  include/linux/fs.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index c95c0807471f..54f3a21668a6 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -3446,7 +3446,7 @@ static inline int check_sticky(struct inode *dir,
>> struct inode *inode)
>>  static inline void inode_has_no_xattr(struct inode *inode)
>>  {
>>  	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & SB_NOSEC))
>> -		inode->i_flags |= S_NOSEC;
>> +		inode_set_flags(inode, S_NOSEC, S_NOSEC);
>>  }
>>
>>  static inline bool is_root_inode(struct inode *inode)
>> -- 
>> 2.19.1
>>
> 
> 
> 

-- 
Technische Universität Dortmund
Alexander Lochmann                PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16                 phone:  +49.231.7556141
D-44227 Dortmund                  fax:    +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]