[PATCH v2] smb: client: split cached_fid bitfields to avoid shared-byte RMW races

[PATCH v2] smb: client: split cached_fid bitfields to avoid shared-byte RMW races

[Henrique Carvalho]

is_open, has_lease and on_list are stored in the same bitfield byte in
struct cached_fid but are updated in different code paths that may run
concurrently. Bitfield assignments generate byte read–modify–write
operations (e.g. `orb $mask, addr` on x86_64), so updating one flag can
restore stale values of the others.

A possible interleaving is:
    CPU1: load old byte (has_lease=1, on_list=1)
    CPU2: clear both flags (store 0)
    CPU1: RMW store (old | IS_OPEN) -> reintroduces cleared bits

To avoid this class of races, convert these flags to separate bool
fields.

Cc: stable@vger.kernel.org
Fixes: ebe98f1447bbc ("cifs: enable caching of directories for which a lease is held")
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
v1 -> v2: Add Fixes: and Cc: stable tags

 fs/smb/client/cached_dir.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index 1e383db7c3374..5091bf45345e8 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -36,10 +36,10 @@ struct cached_fid {
 	struct list_head entry;
 	struct cached_fids *cfids;
 	const char *path;
-	bool has_lease:1;
-	bool is_open:1;
-	bool on_list:1;
-	bool file_all_info_is_valid:1;
+	bool has_lease;
+	bool is_open;
+	bool on_list;
+	bool file_all_info_is_valid;
 	unsigned long time; /* jiffies of when lease was taken */
 	unsigned long last_access_time; /* jiffies of when last accessed */
 	struct kref refcount;
-- 
2.52.0

Re: [PATCH v2] smb: client: split cached_fid bitfields to avoid shared-byte RMW races

[Shyam Prasad N]

On Tue, Jan 27, 2026 at 9:39 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> is_open, has_lease and on_list are stored in the same bitfield byte in
> struct cached_fid but are updated in different code paths that may run
> concurrently. Bitfield assignments generate byte read–modify–write
> operations (e.g. `orb $mask, addr` on x86_64), so updating one flag can
> restore stale values of the others.
>
> A possible interleaving is:
>     CPU1: load old byte (has_lease=1, on_list=1)
>     CPU2: clear both flags (store 0)
>     CPU1: RMW store (old | IS_OPEN) -> reintroduces cleared bits
>
> To avoid this class of races, convert these flags to separate bool
> fields.
>
> Cc: stable@vger.kernel.org
> Fixes: ebe98f1447bbc ("cifs: enable caching of directories for which a lease is held")
> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
> v1 -> v2: Add Fixes: and Cc: stable tags
>
>  fs/smb/client/cached_dir.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> index 1e383db7c3374..5091bf45345e8 100644
> --- a/fs/smb/client/cached_dir.h
> +++ b/fs/smb/client/cached_dir.h
> @@ -36,10 +36,10 @@ struct cached_fid {
>         struct list_head entry;
>         struct cached_fids *cfids;
>         const char *path;
> -       bool has_lease:1;
> -       bool is_open:1;
> -       bool on_list:1;
> -       bool file_all_info_is_valid:1;
> +       bool has_lease;
> +       bool is_open;
> +       bool on_list;
> +       bool file_all_info_is_valid;
>         unsigned long time; /* jiffies of when lease was taken */
>         unsigned long last_access_time; /* jiffies of when last accessed */
>         struct kref refcount;
> --
> 2.52.0
>
>

Does making them as separate bool fields ensure that compiler does not
optimize them into bitfields anyway?
Ideally, we want to protect these fields with a mutex / spinlock,
which doesn't leave us suspect to such issues.

-- 
Regards,
Shyam

Re: [PATCH v2] smb: client: split cached_fid bitfields to avoid shared-byte RMW races

[Henrique Carvalho]

On Wed, Jan 28, 2026 at 05:03:03PM +0530, Shyam Prasad N wrote:
> On Tue, Jan 27, 2026 at 9:39 PM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> >
> > is_open, has_lease and on_list are stored in the same bitfield byte in
> > struct cached_fid but are updated in different code paths that may run
> > concurrently. Bitfield assignments generate byte read–modify–write
> > operations (e.g. `orb $mask, addr` on x86_64), so updating one flag can
> > restore stale values of the others.
> >
> > A possible interleaving is:
> >     CPU1: load old byte (has_lease=1, on_list=1)
> >     CPU2: clear both flags (store 0)
> >     CPU1: RMW store (old | IS_OPEN) -> reintroduces cleared bits
> >
> > To avoid this class of races, convert these flags to separate bool
> > fields.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: ebe98f1447bbc ("cifs: enable caching of directories for which a lease is held")
> > Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> > v1 -> v2: Add Fixes: and Cc: stable tags
> >
> >  fs/smb/client/cached_dir.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> > index 1e383db7c3374..5091bf45345e8 100644
> > --- a/fs/smb/client/cached_dir.h
> > +++ b/fs/smb/client/cached_dir.h
> > @@ -36,10 +36,10 @@ struct cached_fid {
> >         struct list_head entry;
> >         struct cached_fids *cfids;
> >         const char *path;
> > -       bool has_lease:1;
> > -       bool is_open:1;
> > -       bool on_list:1;
> > -       bool file_all_info_is_valid:1;
> > +       bool has_lease;
> > +       bool is_open;
> > +       bool on_list;
> > +       bool file_all_info_is_valid;
> >         unsigned long time; /* jiffies of when lease was taken */
> >         unsigned long last_access_time; /* jiffies of when last accessed */
> >         struct kref refcount;
> > --
> > 2.52.0
> >
> >
> 
> Does making them as separate bool fields ensure that compiler does not
> optimize them into bitfields anyway?

Fair question, I hadn't thought about it.

However, according to C11 standard, that is not allowed:

        2 Except for bit-fields, objects are composed of contiguous sequences of
        one or more bytes, the number, order, and encoding of which are either
        explicitly specified or implementation-defined. (6.2.6.1)

        15 Within a structure object, the non-bit-field members and the
        units in which bit-fields reside have addresses that increase in
        the order in which they are declared. [...]

> Ideally, we want to protect these fields with a mutex / spinlock,
> which doesn't leave us suspect to such issues.

So having them as separate bool fields should be enough.

-- 
Henrique
SUSE Labs