linux-kernel - Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

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: <CAJfpegtfeCJgzSLOYABTaZ7Hec6JDMHpQtxDzg61jAPJcRZQZA@mail.gmail.com>
Date: Thu, 4 Sep 2025 12:20:46 +0200
From: Miklos Szeredi <miklos@...redi.hu>
To: Luis Henriques <luis@...lia.com>
Cc: Bernd Schubert <bernd@...ernd.com>, Laura Promberger <laura.promberger@...n.ch>, 
	Dave Chinner <david@...morbit.com>, Matt Harvey <mharvey@...ptrading.com>, 
	linux-fsdevel@...r.kernel.org, kernel-dev@...lia.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically
 invalidate expired dentries

On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@...lia.com> wrote:

> +#define HASH_BITS      12

Definitely too large.  My gut feeling gives 5, but it obviously
depends on a lot of factors.

> +               schedule_delayed_work(&dentry_tree_work,
> +                                     secs_to_jiffies(num));

secs_to_jiffues() doesn't check overflow.  Perhaps simplest fix would
be to constrain parameter to unsigned short.

> +MODULE_PARM_DESC(inval_wq,
> +                "Dentries invalidation work queue period in secs (>= 5).");

__stringify(FUSE_DENTRY_INVAL_FREQ_MIN)

> +       if (!inval_wq && RB_EMPTY_NODE(&fd->node))
> +               return;

inval_wq can change to zero, which shouldn't prevent removing from the rbtree.

> +static void fuse_dentry_tree_work(struct work_struct *work)
> +{
> +       struct fuse_dentry *fd;
> +       struct rb_node *node;
> +       int i;
> +
> +       for (i = 0; i < HASH_SIZE; i++) {
> +               spin_lock(&dentry_hash[i].lock);
> +               node = rb_first(&dentry_hash[i].tree);
> +               while (node && !need_resched()) {

Wrong place.

> +                       fd = rb_entry(node, struct fuse_dentry, node);
> +                       if (time_after64(get_jiffies_64(), fd->time)) {
> +                               rb_erase(&fd->node, &dentry_hash[i].tree);
> +                               RB_CLEAR_NODE(&fd->node);
> +                               spin_unlock(&dentry_hash[i].lock);

cond_resched() here instead.

> +                               d_invalidate(fd->dentry);

Okay, so I understand the reasoning: the validity timeout for the
dentry expired, hence it's invalid.  The problem is, this is not quite
right.  The validity timeout says "this dentry is assumed valid for
this period", it doesn't say the dentry is invalid after the timeout.

Doing d_invalidate() means we "know the dentry is invalid", which will
get it off the hash tables, giving it a "(deleted)" tag in proc
strings, etc.  This would be wrong.

What we want here is just get rid of *unused* dentries, which don't
have any reference.  Referenced ones will get revalidated with
->d_revalidate() and if one turns out to be actually invalid, it will
then be invalidated with d_invalidate(), otherwise the timeout will
just be reset.

There doesn't seem to be a function that does this, so new
infrastructure will need to be added to fs/dcache.c.  Exporting
shrink_dentry_list() and to_shrink_list() would suffice, but I wonder
if the helpers should be a little higher level.

> +void fuse_dentry_tree_cleanup(void)
> +{
> +       struct rb_node *n;
> +       int i;
> +
> +       inval_wq = 0;
> +       cancel_delayed_work_sync(&dentry_tree_work);
> +
> +       for (i = 0; i < HASH_SIZE; i++) {

If we have anything in there at module remove, then something is
horribly broken.  A WARN_ON definitely suffices here.

> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -54,6 +54,12 @@
>  /** Frequency (in jiffies) of request timeout checks, if opted into */
>  extern const unsigned long fuse_timeout_timer_freq;
>
> +/*
> + * Dentries invalidation workqueue period, in seconds.  It shall be >= 5

If we have a definition of this constant, please refer to that
definition here too.

> @@ -2045,6 +2045,10 @@ void fuse_conn_destroy(struct fuse_mount *fm)
>
>         fuse_abort_conn(fc);
>         fuse_wait_aborted(fc);
> +       /*
> +        * XXX prune dentries:
> +        * fuse_dentry_tree_prune(fc);
> +        */

No need.

Thanks,
Miklos