lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 5 Aug 2021 11:24:57 +0200
From:   Jan Kara <jack@...e.cz>
To:     Gabriel Krisman Bertazi <krisman@...labora.com>
Cc:     jack@...e.com, amir73il@...il.com, djwong@...nel.org,
        tytso@....edu, david@...morbit.com, dhowells@...hat.com,
        khazhy@...gle.com, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-api@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH v5 05/23] fanotify: Split superblock marks out to a new
 cache

On Wed 04-08-21 12:05:54, Gabriel Krisman Bertazi wrote:
> FAN_FS_ERROR will require an error structure to be stored per mark.
> But, since FAN_FS_ERROR doesn't apply to inode/mount marks, it should
> suffice to only expose this information for superblock marks. Therefore,
> wrap this kind of marks into a container and plumb it for the future.
> 
> Reviewed-by: Amir Goldstein <amir73il@...il.com>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.com>

Looks mostly good, just one nit below:

> @@ -915,6 +916,38 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
>  	return mask & ~oldmask;
>  }
>  
> +static struct fsnotify_mark *fanotify_alloc_mark(struct fsnotify_group *group,
> +						 unsigned int type)
> +{
> +	struct fanotify_sb_mark *sb_mark;
> +	struct fsnotify_mark *mark;
> +
> +	switch (type) {
> +	case FSNOTIFY_OBJ_TYPE_SB:
> +		sb_mark = kmem_cache_zalloc(fanotify_sb_mark_cache, GFP_KERNEL);
> +		if (!sb_mark)
> +			return NULL;
> +		mark = &sb_mark->fsn_mark;
> +		break;
> +
> +	case FSNOTIFY_OBJ_TYPE_INODE:
> +	case FSNOTIFY_OBJ_TYPE_PARENT:
> +	case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> +		mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
> +		break;

It is odd that sb marks are allocated with zalloc while other marks with
alloc. Why is that? It is errorprone to have this different among mark
types as somebody may mistakenly assume a mark is zeroed when it actually is
not. So please either use kmem_cache_alloc() for sb mark as well and zero
out by hand what you need, or do a cleanup patch that uses zalloc across
all of dnotify, inotify, fanotify (I can see kernel/audit_* users already
use zalloc) and drop zeroing from fsnotify_init_mark(). Thanks!

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

Powered by blists - more mailing lists