linux-kernel - Re: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie

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 linux-cve-announce 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]

Message-ID: <1444650.1650549423@warthog.procyon.org.uk>
Date:   Thu, 21 Apr 2022 14:57:03 +0100
From:   David Howells <dhowells@...hat.com>
To:     Jeffle Xu <jefflexu@...ux.alibaba.com>
Cc:     dhowells@...hat.com, linux-cachefs@...hat.com, xiang@...nel.org,
        chao@...nel.org, linux-erofs@...ts.ozlabs.org,
        torvalds@...ux-foundation.org, gregkh@...uxfoundation.org,
        willy@...radead.org, linux-fsdevel@...r.kernel.org,
        joseph.qi@...ux.alibaba.com, bo.liu@...ux.alibaba.com,
        tao.peng@...ux.alibaba.com, gerry@...ux.alibaba.com,
        eguan@...ux.alibaba.com, linux-kernel@...r.kernel.org,
        luodaowen.backend@...edance.com, tianzichen@...ishou.com,
        fannaihao@...du.com, zhangjiachen.jaycee@...edance.com
Subject: Re: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie

Jeffle Xu <jefflexu@...ux.alibaba.com> wrote:

> +	help
> +	  This permits on-demand read mode of cachefiles.  In this mode, when
> +	  cache miss, the cachefiles backend instead of netfs, is responsible
> +	  for fetching data, e.g. through user daemon.

How about:

	help
	  This permits userspace to enable the cachefiles on-demand read mode.
	  In this mode, when a cache miss occurs, responsibility for fetching
	  the data lies with the cachefiles backend instead of with the netfs
	  and is delegated to userspace.

> +	/*
> +	 * 1) Cache has been marked as dead state, and then 2) flush all
> +	 * pending requests in @reqs xarray. The barrier inside set_bit()
> +	 * will ensure that above two ops won't be reordered.
> +	 */

What set_bit()?  What "above two ops"?  And that's not how barriers work; they
provide a partial ordering relative to another pair of barriered ops.

Also, set_bit() can't be relied upon to imply a barrier - see
Documentation/memory-barriers.txt.

> +	if (IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND) &&
> +	    test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags)) {

It might be worth abstracting this into an inline function in internal.h:

	static inline bool cachefiles_in_ondemand_mode(cache)
	{
		return IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND) &&
			test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags)
	}

> +#ifdef CONFIG_CACHEFILES_ONDEMAND

This looks like it ought to be superfluous, given the preceding test - though
I can see why you need it:

> +#ifdef CONFIG_CACHEFILES_ONDEMAND
> +	struct xarray			reqs;		/* xarray of pending on-demand requests */
> +	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
> +	u32				ondemand_id_next;
> +#endif

I'm tempted to say that you should just make them non-conditional.  It's not
like there's likely to be more than one or two cachefiles_cache structs on a
system.

David