Re: [EXTERNAL] Re: [PATCH v5] hfs: update sanity check of the root record

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Tue, 2026-03-31 at 10:12 +0900, Tetsuo Handa wrote:
> On 2026/03/31 6:45, Viacheslav Dubeyko wrote:
> > I've already reviewed this patch. And I am not agree with this suggestion.
> > 
> > We prepare the key with HFSPLUS_ROOT_CNID [1]:
> > 
> > err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> 
> What we are talking about is not hfsplus but hfs.
> I can't catch why you are talking about hfsplus function.

There are a lot of similarity between HFS and HFS+ b-trees functionality. Even
we can have a common code in the form of library shared between HFS and HFS+
code. HFS and HFS+ have pretty the same function names in b-tree
implementations.

> 
> > 
> > The hfs_brec_read() executes the search of the record [2]:
> > 
> > 	res = hfs_brec_find(fd);
> > 	if (res)
> > 		return res;
> > 
> > The hfs_brec_find() should found the record for requested key. And if the found
> > thread record contains not correct CNID, then we can check the found thread
> > record and return error as the result of the search.
> 
> hfs_brec_read() indeed calls hfs_brec_find(). But I can't interpret how to extract
> CNID as of returning from hfs_brec_find().

/* The catalog record for a file */
struct hfs_cat_file {
	s8 type;			/* The type of entry */
	u8 reserved;
	u8 Flags;			/* Flags such as read-only */
	s8 Typ;				/* file version number = 0 */
	struct hfs_finfo UsrWds;	/* data used by the Finder */
	__be32 FlNum;			/* The CNID */
	__be16 StBlk;			/* obsolete */
	__be32 LgLen;			/* The logical EOF of the data fork*/
	__be32 PyLen;			/* The physical EOF of the data fork */
	__be16 RStBlk;			/* obsolete */
	__be32 RLgLen;			/* The logical EOF of the rsrc fork */
	__be32 RPyLen;			/* The physical EOF of the rsrc fork */
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modified date */
	__be32 BkDat;			/* The last backup date */
	struct hfs_fxinfo FndrInfo;	/* more data for the Finder */
	__be16 ClpSize;			/* number of bytes to allocate
					   when extending files */
	hfs_extent_rec ExtRec;		/* first extent record
					   for the data fork */
	hfs_extent_rec RExtRec;		/* first extent record
					   for the resource fork */
	u32 Resrv;			/* reserved by Apple */
} __packed;

/* the catalog record for a directory */
struct hfs_cat_dir {
	s8 type;			/* The type of entry */
	u8 reserved;
	__be16 Flags;			/* flags */
	__be16 Val;			/* Valence: number of files and
					   dirs in the directory */
	__be32 DirID;			/* The CNID */
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modification date */
	__be32 BkDat;			/* The last backup date */
	struct hfs_dinfo UsrInfo;	/* data used by the Finder */
	struct hfs_dxinfo FndrInfo;	/* more data used by Finder */
	u8 Resrv[16];			/* reserved by Apple */
} __packed;

/* the catalog record for a thread */
struct hfs_cat_thread {
	s8 type;			/* The type of entry */
	u8 reserved[9];			/* reserved by Apple */
	__be32 ParID;			/* CNID of parent directory */
	struct hfs_name CName;		/* The name of this entry */
}  __packed;

/* A catalog tree record */
typedef union hfs_cat_rec {
	s8 type;			/* The type of entry */
	struct hfs_cat_file file;
	struct hfs_cat_dir dir;
	struct hfs_cat_thread thread;
} hfs_cat_rec;

Every record starts with type. And anyone can easily retrieve the type of
record. And, then, you can extract the CNID.

> 
>   int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
>   {
>   	int res;
>   
>   	res = hfs_brec_find(fd);
>   	if (res)
>   		return res;
>   	if (fd->entrylength > rec_len)
>   		return -EINVAL;
>   	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
>   	return 0;
>   }
> 
> Since hfs_brec_read() doesn't know which type of struct (one of "struct hfs_cat_file",
> "struct hfs_cat_dir" or "struct hfs_cat_thread") does the caller of hfs_brec_read()
> want to read, I don't think hfs_brec_read() can tell whether the CNID is correct.

Please, check the HFS's on-disk layout.

> 
> I am waiting for your response on
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_9f66743b-2D70e0-2D4886-2D884e-2D5203f5c02ed8-40I-2Dlove.SAKURA.ne.jp&d=DwICaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=V9I6gMC1If2D6irQzVVC1M0wKQW-kbErffC9npM7z-fyjkutPS0YOARfqxRsyUdc&s=MAn71bGJ4-F3OEEhjru0azzQc62bUcvfCsk1VndcjC4&e= 
> where the caller of hfs_brec_read() can tell whether the CNID is correct. But such
> change is a matter of preference.
> 
> You can respond with your patch which will be much faster.
> 

Sorry, I am busy with other HFS/HFS+ issues.

Thanks,
Slava.