Re: [PATCH] hfs: avoid reusing occupied CNIDs after deletion

[Viacheslav Dubeyko]

On Wed, 2026-10-21 at 11:31 -0400, Hengyu Liang wrote:
> Hi Slava,
> 
> On Mon, Oct 19, 2026 at 04:29:16PM -0700, Viacheslav Dubeyko wrote:
> > Frankly speaking, I don't see how your modification can fix
> > anything.
> > From my point of view, the current logic is correct. Probably, the
> > call of this method is not in the proper place.
> 
> Sorry, my description was not clear enough. Here is the reproducer
> with the actual CNIDs, on a freshly formatted volume:
> 
>     operation    allocated CNID    next_id afterwards
>     mkdir z            16                 17
>     create a           17                 18
>     create b           18                 19
> 
> rm b calls hfs_correct_next_unused_CNID() with cnid 18 and next_id
> 19,
> after b's file and thread records have been removed. The catalog leaf
> records are then, in key order:
> 
>     key                type           CNID
>     (1, "untitled")    dir              2
>     (2, "")            dir thread
>     (2, "a")           file            17
>     (2, "z")           dir             16
>     (16, "")           dir thread
>     (17, "")           file thread
> 

I think I see your point. So, I think that fix is much simpler. We need
to add the thread records processing here [1]:

			if (rec.type == HFS_CDR_DIR) {
				found_cnid =
be32_to_cpu(rec.dir.DirID);
				hfs_dbg("found_cnid %u\n",
found_cnid);
				hfs_set_next_unused_CNID(sb, cnid,
found_cnid);
				hfs_bnode_put(node);
				return 0;
			} else if (rec.type == HFS_CDR_FIL) {
				found_cnid =
be32_to_cpu(rec.file.FlNum);
				hfs_dbg("found_cnid %u\n",
found_cnid);
				hfs_set_next_unused_CNID(sb, cnid,
found_cnid);
				hfs_bnode_put(node);
				return 0;
			}

#define HFS_CDR_DIR		0x01	/* folder (directory) */
#define HFS_CDR_FIL		0x02	/* file */
#define HFS_CDR_THD		0x03	/* folder (directory) thread
*/
#define HFS_CDR_FTH		0x04	/* file thread */

The current logic simply misses the HFS_CDR_THD and HFS_CDR_FTH cases
processing.

Thanks,
Slava.

> The backward scan skips the two thread records at the end and stops
> at
> z, so it changes next_id from 19 to 17, although a still uses 17. The
> next create gets CNID 17, hfs_cat_create() finds a's thread record
> and
> returns -EEXIST. The correct value is 18.
> 
> So the call site is not the problem: the records are already gone
> when
> the correction runs, and calling it earlier or later would not change
> the result, because z still sorts last. The problem is that file and
> directory records are ordered by (parent ID, name), not by their own
> CNID, so the last one need not hold the largest remaining CNID. The
> patch keeps the call where it is, so the drNxtCNID correction that
> generic/736 needs stays, and only makes the scan take the maximum
> over
> all of them.
> 
> The unpatched run in my previous mail matches this: the first create
> of c failed, and the retry got CNID 18, so the failed attempt had
> been
> given 17.
> 
> > I don't quite follow why do we need to change this check? If cnid +
> > 1
> > is smaller than the maximum one, then cnid + 1 is in use yet. We
> > don't need to execute any additional logic. What am I missing here?
> 
> Nothing; both checks return early in that case. != additionally
> returns early when cnid + 1 > next_id, where the cmpxchg in
> hfs_set_next_unused_CNID() cannot succeed anyway. It is not needed
> for
> the fix (in the example both checks go on to the correction), and I
> can drop it in v2.
> 
> > Why do we need to check something for previous one? If the bigger
> > cnid is in use, then we don't need to do anything here.
> 
> The lookup only runs when cnid + 1 == next_id, i.e. the deleted CNID
> was the largest one allocated, so no bigger CNID is in use. If
> cnid - 1 still exists, it is the largest remaining CNID, and next_id
> can be set to cnid without walking the catalog: in the example,
> deleting 18 finds 17 and sets next_id to 18. Because the fallback now
> walks the whole catalog instead of stopping at the first record, this
> keeps the common case cheap. Only if the lookup returns -ENOENT does
> the patch scan all records, which also covers files without the
> optional thread record.
> 
> Thanks,
> Hengyu

[1] https://elixir.bootlin.com/linux/v7.3/source/fs/hfs/catalog.c#L349