linux-kernel - Re: [PATCH v5 bpf-next 0/5] bpf path iterator

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] [day] [month] [year] [list]
Message-ID: <707D80F3-9EEC-4108-8F8D-0BE069133E35@meta.com>
Date: Fri, 27 Jun 2025 00:21:23 +0000
From: Song Liu <songliubraving@...a.com>
To: NeilBrown <neil@...wn.name>
CC: Tingmao Wang <m@...wtm.org>,
        Mickaël Salaün
	<mic@...ikod.net>,
        Song Liu <song@...nel.org>,
        "bpf@...r.kernel.org"
	<bpf@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org"
	<linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-security-module@...r.kernel.org"
	<linux-security-module@...r.kernel.org>,
        "brauner@...nel.org"
	<brauner@...nel.org>,
        Kernel Team <kernel-team@...a.com>,
        "andrii@...nel.org"
	<andrii@...nel.org>,
        "eddyz87@...il.com" <eddyz87@...il.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net"
	<daniel@...earbox.net>,
        "martin.lau@...ux.dev" <martin.lau@...ux.dev>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "jack@...e.cz"
	<jack@...e.cz>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>,
        Günther Noack <gnoack@...gle.com>
Subject: Re: [PATCH v5 bpf-next 0/5] bpf path iterator



> On Jun 26, 2025, at 3:51 PM, NeilBrown <neil@...wn.name> wrote:

[...]

>> Unfortunately, the BPF use case is more complicated. In some cases, 
>> the callback function cannot be call in rcu critical sections. For 
>> example, the callback may need to read xatter. For these cases, we
>> we cannot use RCU walk at all.
> 
> I really think you should stop using the terms RCU walk and ref-walk.  I
> think they might be focusing your thinking in an unhelpful direction.
> 
> The key issue about reading xattrs is that it might need to sleep.
> Focusing on what might need to sleep and what will never need to sleep
> is a useful approach - the distinction is wide spread in the kernel and
> several function take a flag indicating if they are permitted to sleep,
> or if failure when sleeping would be required.
> 
> So your above observation is better described as 
> 
>   The vfs_walk_ancestors() API has an (implicit) requirement that the
>   callback mustn't sleep.  This is a problem for some use-cases
>   where the call back might need to sleep - e.g. for accessing xattrs.
> 
> That is a good and useful observation.  I can see three possibly
> responses:
> 
> 1/ Add a vfs_walk_ancestors_maysleep() API for which the callback is
>   always allowed to sleep.  I don't particularly like this approach.
> 
> 2/ Use repeated calls to vfs_walk_parent() when the handling of each
>   ancestor might need to sleep.  I see no problem with supporting both
>   vfs_walk_ancestors() and vfs_walk_parent().  There is plenty of
>   precedent for having different  interfaces for different use cases.

I prefer option 2. 

> 
> 3/ Extend vfs_walk_ancestors() to pass a "may sleep" flag to the callback.
>   If the callback finds that it needs to sleep but that "may sleep"
>   isn't set, it returns some well known status, like -EWOULDBLOCK (or
>   -ECHILD).  It can expect to be called again but with "may sleep" set.
>   This is my preferred approach. There is precedent with the
>   d_revalidate callbacks which works like this.
>   I suspect that accessing xattrs might often be possible without
>   sleeping.  It is conceivable that we could add a "may sleep" argument
>   to vfs_getxattr() so that it could still often be used without
>   requiring vfs_walk_ancestors() to permit sleeping.
>   This would almost certainly require a clear demonstration that 
>   there was a performance cost in not having the option of non-sleeping
>   vfs_getxattr().

For built-in kernel code, I can see this works. However, I don’t see 
why it is necessary to introduce the extra complexity of -EWOULDBLOCK, 
and vfs_get_xattr_cannot_sleep, etc. A separate step-by-step walking
API is much cleaner.

> 
>>> I strongly suggest you stop thinking about rcu-walk vs ref-walk.  Think
>>> about the needs of your code.  If you need a high-performance API, then
>>> ask for a high-performance API, don't assume what form it will take or
>>> what the internal implementation details will be.
>> 
>> At the moment, we need a ref-walk API on the BPF side. The RCU walk
>> is a totally separate topic.
> 
> Do you mean "we need step-by-step walking" or do you mean "we need to
> potentially sleep for each ancestor"?  These are conceptually different
> requirements, but I cannot tell which you mean when you talk about "RCU
> walk”.

To be extra clear, I mean we need "step-by-step and 
take-reference-on-each-step walking”, for existing use cases. 

In the future, if it is possible to have a “do-not-take-reference, 
cannot-sleep, callback-based walking”. We may try to use that for 
some use cases. But that won’t replace step-by-step walking for 
all users. 

Thanks,
Song