Re: [PATCH v2] hfsplus: annotate extents_lock nesting to silence lockdep false positive

[Viacheslav Dubeyko]

On Fri, 2026-10-02 at 23:18 +0400, Kullanılmıyor Pasif wrote:
> Hi Slava,
> Thanks. You're right that tree->cnid and inode->i_ino match for the
> special inodes. hfsplus_btree_lock_class() itself can't be reused as-
> is, though: it takes a struct hfs_btree * and BUG()s on any CNID that
> isn't one of the three B-trees (CATALOG/EXTENTS/ATTR), while
> extents_lock is taken on regular inodes and on the allocation file,
> which are not B-trees.

Technically speaking, we can rework hfsplus_btree_lock_class() to be
applicable for both cases. This was my point.

> Looking more carefully, there is only one actual extents_lock
> nesting: a regular inode's extents_lock is held in
> hfsplus_file_truncate()/hfsplus_file_extend() while block alloc/free
> reads sbi->alloc_file (hfsplus_block_free() -> read_mapping_page() ->
> hfsplus_get_block() on the allocation file). The catalog/attr inodes
> are not taken nested under a regular inode's extents_lock (the
> extents-overflow inode returns early in hfsplus_get_block(), and the
> catalog/attr trees are reached under their own tree_lock).
> So the parallel enum + HFSPLUS_FIRSTUSER_CNID check is overkill (and,
> as you note, FIRSTUSER also covers folders). I'll simplify to a
> single distinction: give only the allocation file's extents_lock a
> separate subclass, leaving every other inode at the default. That
> removes the duplication and the file/folder ambiguity.
> Does that direction look right to you? I'll send v3 accordingly and
> fold the same into the HFS side.

I am not sure that I completely follow to your vision. Simply, send
another version of the patch and we will discuss it.

Thanks,
Slava.

> Thanks,
> Mahmut
> 
> Viacheslav Dubeyko <slava@xxxxxxxxxxx>, 2 Eki 2026 Cum, 22:10
> tarihinde şunu yazdı:
> > On Thu, 2026-10-01 at 23:53 +0200, Mahmut Emin Kurhan wrote:
> > > lockdep reports a possible recursive locking on HFSPLUS_I(inode)-
> > > > extents_lock
> > > when unlinking a file on a crafted image: hfsplus_file_truncate()
> > > holds a
> > > regular file's extents_lock and, while freeing blocks, reads the
> > > allocation
> > > file, whose get_block() takes that file's own extents_lock. They
> > > are
> > > distinct
> > > inodes of the same lock class taken in a fixed order, so it is
> > > not a
> > > real
> > > deadlock, only a missing nesting annotation.
> > > 
> > 
> > As far as I can see, we have the same issue for HFS code.
> > 
> > > Give the special inodes (allocation, attributes, catalog)
> > > distinct
> > > extents_lock subclasses and take the lock with
> > > mutex_lock_nested(),
> > > mirroring
> > > hfsplus_btree_lock_class(). Regular files (CNID >=
> > > HFSPLUS_FIRSTUSER_CNID) keep
> > > their own subclass so a genuine regular-vs-regular recursion is
> > > still
> > > detected;
> > > the remaining reserved inodes use a separate subclass instead of
> > > being treated
> > > as regular files.
> > > 
> > > Found via coverage-guided fuzzing (syzkaller + lockdep) by
> > > Noroxi.
> > > 
> > > Signed-off-by: Mahmut Emin Kurhan <guvenlik@xxxxxxxxxx>
> > > ---
> > > v2:
> > >  - shorten the comment (Slava Dubeyko)
> > >  - rename HFSPLUS_EXTENTS_LOCK_NORMAL to _REGULAR_FILE (Slava
> > > Dubeyko)
> > >  - use HFSPLUS_FIRSTUSER_CNID so reserved inodes are not treated
> > > as
> > >    regular files; add a separate _OTHER subclass (Slava Dubeyko)
> > >  - hfsplus_extents_lock_class() signature is 74 cols (< 80)
> > >  fs/hfsplus/extents.c    | 12 ++++++++----
> > >  fs/hfsplus/hfsplus_fs.h | 26 ++++++++++++++++++++++++++
> > >  fs/hfsplus/xattr.c      |  3 ++-
> > >  3 files changed, 36 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> > > index eb7c11524d..54a05eaa8f 100644
> > > --- a/fs/hfsplus/extents.c
> > > +++ b/fs/hfsplus/extents.c
> > > @@ -150,7 +150,8 @@ int hfsplus_ext_write_extent(struct inode
> > > *inode)
> > >  {
> > >       int res;
> > >  
> > > -     mutex_lock(&HFSPLUS_I(inode)->extents_lock);
> > > +     mutex_lock_nested(&HFSPLUS_I(inode)->extents_lock,
> > > +                       hfsplus_extents_lock_class(inode));
> > >       res = hfsplus_ext_write_extent_locked(inode);
> > >       mutex_unlock(&HFSPLUS_I(inode)->extents_lock);
> > >  
> > > @@ -261,7 +262,8 @@ int hfsplus_get_block(struct inode *inode,
> > > sector_t iblock,
> > >       if (inode->i_ino == HFSPLUS_EXT_CNID)
> > >               return -EIO;
> > >  
> > > -     mutex_lock(&hip->extents_lock);
> > > +     mutex_lock_nested(&hip->extents_lock,
> > > +                       hfsplus_extents_lock_class(inode));
> > >  
> > >       /*
> > >        * hfsplus_ext_read_extent will write out a cached extent
> > > into
> > > @@ -454,7 +456,8 @@ int hfsplus_file_extend(struct inode *inode,
> > > bool
> > > zeroout)
> > >               return -ENOSPC;
> > >       }
> > >  
> > > -     mutex_lock(&hip->extents_lock);
> > > +     mutex_lock_nested(&hip->extents_lock,
> > > +                       hfsplus_extents_lock_class(inode));
> > >       if (hip->alloc_blocks == hip->first_blocks)
> > >               goal = hfsplus_ext_lastblock(hip->first_extents);
> > >       else {
> > > @@ -576,7 +579,8 @@ void hfsplus_file_truncate(struct inode
> > > *inode)
> > >       blk_cnt = (inode->i_size + HFSPLUS_SB(sb)->alloc_blksz - 1)
> > > > > 
> > >                       HFSPLUS_SB(sb)->alloc_blksz_shift;
> > >  
> > > -     mutex_lock(&hip->extents_lock);
> > > +     mutex_lock_nested(&hip->extents_lock,
> > > +                       hfsplus_extents_lock_class(inode));
> > >  
> > >       alloc_cnt = hip->alloc_blocks;
> > >       if (blk_cnt == alloc_cnt)
> > > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> > > index 916e6552e3..7e2411b8f1 100644
> > > --- a/fs/hfsplus/hfsplus_fs.h
> > > +++ b/fs/hfsplus/hfsplus_fs.h
> > > @@ -37,6 +37,15 @@ enum hfsplus_btree_mutex_classes {
> > >       ATTR_BTREE_MUTEX,
> > >  };
> > >  
> > > +/* lockdep subclasses for extents_lock: special inodes nest
> > > under
> > > regular files */
> > > +enum hfsplus_extents_mutex_classes {
> > > +     HFSPLUS_EXTENTS_LOCK_REGULAR_FILE,
> > > +     HFSPLUS_EXTENTS_LOCK_CATALOG,
> > > +     HFSPLUS_EXTENTS_LOCK_ALLOC,
> > > +     HFSPLUS_EXTENTS_LOCK_ATTR,
> > > +     HFSPLUS_EXTENTS_LOCK_OTHER,
> > > +};
> > 
> > After more careful consideration, I think it looks like a
> > duplication.
> > We already have:
> > 
> > enum hfsplus_btree_mutex_classes {
> >         CATALOG_BTREE_MUTEX,
> >         EXTENTS_BTREE_MUTEX,
> >         ATTR_BTREE_MUTEX,
> > };
> > 
> > static inline enum hfsplus_btree_mutex_classes
> > hfsplus_btree_lock_class(struct hfs_btree *tree)
> > {
> >         enum hfsplus_btree_mutex_classes class;
> > 
> >         switch (tree->cnid) {
> >         case HFSPLUS_CAT_CNID:
> >                 class = CATALOG_BTREE_MUTEX;
> >                 break;
> >         case HFSPLUS_EXT_CNID:
> >                 class = EXTENTS_BTREE_MUTEX;
> >                 break;
> >         case HFSPLUS_ATTR_CNID:
> >                 class = ATTR_BTREE_MUTEX;
> >                 break;
> >         default:
> >                 BUG();
> >         }
> >         return class;
> > }
> > 
> > Why we cannot reuse this already existing logic? Suggested logic
> > uses
> > the inode->i_ino. But tree->cnid and inode->i_ino should be the
> > same.
> > Maybe, we simply need to rework existing logic and reuse it? What
> > do
> > you think?
> > 
> > > +
> > >  /* An HFS+ BTree held in memory */
> > >  struct hfs_btree {
> > >       struct super_block *sb;
> > > @@ -568,6 +577,23 @@ hfsplus_btree_lock_class(struct hfs_btree
> > > *tree)
> > >       return class;
> > >  }
> > >  
> > > +static inline unsigned int hfsplus_extents_lock_class(struct
> > > inode
> > > *inode)
> > > +{
> > > +     if (inode->i_ino >= HFSPLUS_FIRSTUSER_CNID)
> > > +             return HFSPLUS_EXTENTS_LOCK_REGULAR_FILE;
> > 
> > The HFSPLUS_FIRSTUSER_CNID coudl be as file as folder. Please, take
> > a
> > look into my above comments. Maybe, we don't a new function at all.
> > 
> > Thanks,
> > Slava.
> > 
> > > +
> > > +     switch (inode->i_ino) {
> > > +     case HFSPLUS_CAT_CNID:
> > > +             return HFSPLUS_EXTENTS_LOCK_CATALOG;
> > > +     case HFSPLUS_ALLOC_CNID:
> > > +             return HFSPLUS_EXTENTS_LOCK_ALLOC;
> > > +     case HFSPLUS_ATTR_CNID:
> > > +             return HFSPLUS_EXTENTS_LOCK_ATTR;
> > > +     default:
> > > +             return HFSPLUS_EXTENTS_LOCK_OTHER;
> > > +     }
> > > +}
> > > +
> > >  static inline
> > >  bool is_bnode_offset_valid(struct hfs_bnode *node, u32 off)
> > >  {
> > > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> > > index 21a1c196c7..7bf4ba8f18 100644
> > > --- a/fs/hfsplus/xattr.c
> > > +++ b/fs/hfsplus/xattr.c
> > > @@ -264,7 +264,8 @@ static int
> > > hfsplus_create_attributes_file(struct
> > > super_block *sb)
> > >                                                   sbi-
> > > >sect_count,
> > >                                                  
> > > HFSPLUS_ATTR_CNID);
> > >  
> > > -     mutex_lock(&hip->extents_lock);
> > > +     mutex_lock_nested(&hip->extents_lock,
> > > +                       hfsplus_extents_lock_class(attr_file));
> > >       hip->clump_blocks = clump_size >> sbi->alloc_blksz_shift;
> > >       mutex_unlock(&hip->extents_lock);
> > >