On Thu, 2025-08-28 at 12:35 +0000, 杨晨志 wrote:
> 在 2025/8/28 4:04, Viacheslav Dubeyko 写道:
> > Frankly speaking, I don't think that reworking this method for
> > returning the error code is necessary. Currently, we return the length
> > value (u16) and we can return U16_MAX as for off as for len for the
> > case of incorrect offset or erroneous logic. We can treat U16_MAX as
> > error condition and we can check off and len for this value. Usually,
> > HFS b-tree node has 512 bytes in size and as offset as length cannot be
> > equal to U16_MAX (or bigger). And we don't need to change the input and
> > output arguments if we will check for U16_MAX value.
>
> Using U16_MAX as the error return value is reasonable. This change also
> applies to hfsplus, since the maximum node_size in hfsplus is 32768
> (0x8000), which is less than U16_MAX. Adopting this approach helps avoid
> extensive interface changes. I agree with your point.
>
> > > diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> > > index e8cd1a31f247..b0bbaf016b8d 100644
> > > --- a/fs/hfs/bnode.c
> > > +++ b/fs/hfs/bnode.c
> > > @@ -57,26 +57,16 @@ int check_and_correct_requested_length(struct
> > > hfs_bnode *node, int off, int len)
> > > return len;
> > > }
> > >
> > > -void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int
> > > len)
> > > +int __hfs_bnode_read(struct hfs_bnode *node, void *buf, u16 off, u16
> > > len)
> >
> > I don't follow why do we need to introduce __hfs_bnode_read(). One
> > method for all is enough. And I think we still can use only
> > hfs_bnode_read(). Because, we can initialize the buffer by 0x00 or 0xFF
> > in the case we cannot read if offset or length are invalid. Usually,
> > every method checks (or should) check the returning value of
> > hfs_bnode_read().
> >
> > > {
> > > struct page *page;
> > > int pagenum;
> > > int bytes_read;
> > > int bytes_to_read;
> > >
> > > - if (!is_bnode_offset_valid(node, off))
> > > - return;
> > > -
> > > - if (len == 0) {
> > > - pr_err("requested zero length: "
> > > - "NODE: id %u, type %#x, height %u, "
> > > - "node_size %u, offset %d, len %d\n",
> > > - node->this, node->type, node->height,
> > > - node->tree->node_size, off, len);
> > > - return;
> > > - }
> > > -
> > > - len = check_and_correct_requested_length(node, off, len);
> > > + /* len = 0 is invalid: prevent use of an uninitalized
> > > buffer*/
> > > + if (!len || !hfs_off_and_len_is_valid(node, off, len))
> > > + return -EINVAL;
> > >
> > > off += node->page_offset;
> > > pagenum = off >> PAGE_SHIFT;
> > > @@ -93,6 +83,47 @@ void hfs_bnode_read(struct hfs_bnode *node, void
> > > *buf, int off, int len)
> > > pagenum++;
> > > off = 0; /* page offset only applies to the first
> > > page */
> > > }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __hfs_bnode_read_u16(struct hfs_bnode *node, u16* buf,
> > > u16 off)
> >
> > I don't see the point to introduce another version of method because we
> > can return U16_MAX as invalid value.
> >
> > > +{
> > > + __be16 data;
> > > + int res;
> > > +
> > > + res = __hfs_bnode_read(node, (void*)(&data), off, 2);
> > > + if (res)
> > > + return res;
> > > + *buf = be16_to_cpu(data);
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +static int __hfs_bnode_read_u8(struct hfs_bnode *node, u8* buf, u16
> > > off)
> >
> > And we can return U8_MAX as invalid value here too.
>
> My original plan was to add a return value to hfs_bnode_read(). However,
> since modifying this interface would require changes across dozens of
> call sites—which seems too extensive—and introducing a return value
> might necessitate additional error handling in certain functions (e.g.,
> hfs_bnode_split()), I wasn’t entirely sure how to proceed.
>
> Therefore, I created __hfs_bnode_read() -- a function that behaves
> identically to hfs_bnode_read but includes a return value—as a way to
> experiment and facilitate discussion. I don’t think we ultimately need
> an additional function; its purpose is strictly to help evaluate, within
> this RFC patch, whether adding a return value to hfs_bnode_read is
> desirable and whether the approach taken in __hfs_bnode_read() would be
> suitable.
>
> I agree that we should check return values at every call site—there are
> too many issues in HFS caused by missing checks. However, I’m a bit
> confused by the suggestion to validate the hfs_bnode_read buffer
> directly. If the buffer is a structured type (such as btree_key),
> detecting errors becomes more challenging. In such cases, we may need to
> rely on methods like memcmp() or memchr_inv(). From that perspective,
> adding a return value to hfs_bnode_read seems an easier way.
>
> If you have a lightweight method to quickly verify the validity of a
> read without introducing a return value, please let me know. I can
> follow up with a new patch to address it.
My initial idea was to check the fields of the record. For example, like this
[1]:
hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, fd.entrylength);
if (entry.type != HFS_CDR_THD) {
pr_err("bad catalog folder thread\n");
err = -EIO;
goto out;
}
However, unfortunately, we also have such patterns [2]:
hfs_bnode_read(node, &node_desc, 0, sizeof(node_desc));
node_desc.next = cpu_to_be32(node->next);
node_desc.num_recs = cpu_to_be16(node->num_recs);
hfs_bnode_write(node, &node_desc, 0, sizeof(node_desc));
Of course, technically speaking, it is possible try to check the record's fields
but we could miss something, it is complicated and it could be more compute-
intensive. So, it is much simpler simply to check the returned error. The
question here: how to minimize the required changes? So, returning error code
from hfs_bnode_read() could be the simplest solution.
However, I don;t completely agree with hfs_off_and_len_is_valid() logic:
+static inline
+bool hfs_off_and_len_is_valid(struct hfs_bnode *node, u16 off, u16 len)
+{
+ bool ret = true;
+ if (off > node->tree->node_size ||
+ off + len > node->tree->node_size)
+ ret = false;
+
+ if (!ret) {
+ pr_err("requested invalid offset: "
+ "NODE: id %u, type %#x, height %u, "
+ "node_size %u, offset %u, length %u\n",
+ node->this, node->type, node->height,
+ node->tree->node_size, off, len);
+ }
+
+ return ret;
+}
If offset is out of node size, then, of course, we cannot read anything. But if
the offset is inside node and (offset + length) is bigger than node size, then
we can simply correct the length and read anyway. The question here: has buffer
enough allocated memory to receive the read data? But this check is should be
done by caller. Potentially, it is possible to receive a granularity size of
requested metadata structure and to compare with the requested length.
Otherwise, we cannot make any other reasonable conclusion.
As far as I can see, we have around 25 calls of hfs_bnode_read() and mostly
functions are ready to return error. So, we can carefully rework this logic.
The hfs_brec_lenoff(), hfs_bnode_read_u16() can return U16_MAX and
hfs_bnode_read_u8() can return U8_MAX. The hfs_bnode_read_key() is under
question yet. Should we return error code here or calling memset() will be
enough? I think hfs_bnode_dump() doesn't require significant rework because it
is mostly debugging function and it should dump as much as possible nevertheless
of detected errors.
Thanks,
Slava.
[1] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/hfs/dir.c#L82
[2] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/hfs/brec.c#L327
> >
