* [PATCH] Improved hfsplus in the kernel
@ 2025-02-25 20:30 Pranjal Prasad
2025-02-25 20:59 ` Al Viro
0 siblings, 1 reply; 2+ messages in thread
From: Pranjal Prasad @ 2025-02-25 20:30 UTC (permalink / raw)
To: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 291 bytes --]
Hi,
Please find attached the patch to improve the HFS+ filesystem support in
the kernel. I have not done much work, but as I require HFS, HFS+, and
APFS in Linux, I decided to maintain it. Please allow me to be the
maintainer of HFS and HFS+ in the kernel.
Best regards,
Pranjal Prasad
[-- Attachment #2: 0001-Improved-hfsplus-in-the-kernel.patch --]
[-- Type: text/x-patch, Size: 32677 bytes --]
From 201b5513650ec86b87a64fa584013da8080e9ee3 Mon Sep 17 00:00:00 2001
From: Pranjal Prasad <prasadpranjal213@gmail.com>
Date: Wed, 26 Feb 2025 01:51:09 +0530
Subject: [PATCH] Improved hfsplus in the kernel
---
fs/hfsplus/attributes.c | 113 +++++++++++++-----
fs/hfsplus/bfind.c | 153 ++++++++++++++++---------
fs/hfsplus/btree.c | 223 +++++++++++++++++++++---------------
fs/hfsplus/xattr_security.c | 50 +++++---
4 files changed, 353 insertions(+), 186 deletions(-)
diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index eeebe80c6be4..81d64944b145 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -5,6 +5,9 @@
* Vyacheslav Dubeyko <slava@dubeyko.com>
*
* Handling of records in attributes tree
+ *
+ * Pranjal Prasad <prasadpranjal213@gmail.com>
+ * Improved and added comments
*/
#include "hfsplus_fs.h"
@@ -79,91 +82,149 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
return 0;
}
-
+/*
+ * Allocate memory for an attribute entry from the attribute tree cache.
+ * This function uses the kernel's memory cache system to allocate memory
+ * for an attribute entry.
+ */
hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
{
- return kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
+ return kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL); // Allocates memory from the cache.
}
+/*
+ * Free an attribute entry back to the attribute tree cache.
+ * This function releases memory allocated for an attribute entry back to
+ * the kernel's memory cache system.
+ */
void hfsplus_destroy_attr_entry(hfsplus_attr_entry *entry)
{
- if (entry)
- kmem_cache_free(hfsplus_attr_tree_cachep, entry);
+ if (entry) // Check if the entry is valid (non-NULL).
+ kmem_cache_free(hfsplus_attr_tree_cachep, entry); // Frees the entry back to the cache.
}
+/*
+ * Constant for invalid attribute record.
+ * Used when the record type is invalid or when an error occurs during record creation.
+ */
#define HFSPLUS_INVALID_ATTR_RECORD -1
+/*
+ * Builds an attribute record in the provided entry based on the record type.
+ * Depending on the record type, this function either initializes the entry for
+ * a specific attribute type or returns an error code if the type is unsupported.
+ *
+ * record_type: The type of the record (e.g., fork data, extents, inline data).
+ * cnid: The CNID of the entry (used for file identification).
+ * value: The value to store in the entry (if applicable).
+ * size: The size of the value.
+ *
+ * Returns: The size of the record, or HFSPLUS_INVALID_ATTR_RECORD on error.
+ */
static int hfsplus_attr_build_record(hfsplus_attr_entry *entry, int record_type,
- u32 cnid, const void *value, size_t size)
+ u32 cnid, const void *value, size_t size)
{
+ // Handle fork data attributes (Mac OS X supports only inline data for forks)
if (record_type == HFSPLUS_ATTR_FORK_DATA) {
/*
- * Mac OS X supports only inline data attributes.
- * Do nothing
+ * Mac OS X supports only inline data attributes for forks.
+ * No action needed here, just initialize the entry with zeros.
*/
memset(entry, 0, sizeof(*entry));
- return sizeof(struct hfsplus_attr_fork_data);
+ return sizeof(struct hfsplus_attr_fork_data); // Return the size of the fork data record.
+
+ // Handle extents attributes (again, only inline data is supported)
} else if (record_type == HFSPLUS_ATTR_EXTENTS) {
/*
- * Mac OS X supports only inline data attributes.
- * Do nothing.
+ * Mac OS X supports only inline data attributes for extents.
+ * No action needed here, just initialize the entry with zeros.
*/
memset(entry, 0, sizeof(*entry));
- return sizeof(struct hfsplus_attr_extents);
+ return sizeof(struct hfsplus_attr_extents); // Return the size of the extents record.
+
+ // Handle inline data attributes
} else if (record_type == HFSPLUS_ATTR_INLINE_DATA) {
u16 len;
+ // Initialize the entry for inline data attributes
memset(entry, 0, sizeof(struct hfsplus_attr_inline_data));
- entry->inline_data.record_type = cpu_to_be32(record_type);
+ entry->inline_data.record_type = cpu_to_be32(record_type); // Set the record type in CPU-to-BE format.
+
+ // If the size is within the allowed range for inline data, proceed to set the length.
if (size <= HFSPLUS_MAX_INLINE_DATA_SIZE)
len = size;
else
- return HFSPLUS_INVALID_ATTR_RECORD;
- entry->inline_data.length = cpu_to_be16(len);
+ return HFSPLUS_INVALID_ATTR_RECORD; // Return an error if the size exceeds the limit.
+
+ // Set the length of the inline data attribute (in CPU-to-BE format)
+ entry->inline_data.length = cpu_to_be16(len);
+
+ // Copy the raw bytes of the data into the entry (memory location for the data).
memcpy(entry->inline_data.raw_bytes, value, len);
- /*
- * Align len on two-byte boundary.
- * It needs to add pad byte if we have odd len.
- */
+
+ // Ensure the length is aligned to a two-byte boundary. If the length is odd, we add a padding byte.
len = round_up(len, 2);
- return offsetof(struct hfsplus_attr_inline_data, raw_bytes) +
- len;
- } else /* invalid input */
- memset(entry, 0, sizeof(*entry));
- return HFSPLUS_INVALID_ATTR_RECORD;
+ // Return the size of the record, considering the padding applied to the raw bytes.
+ return offsetof(struct hfsplus_attr_inline_data, raw_bytes) + len;
+ } else {
+ // If the record type is invalid, clear the entry and return the invalid record constant.
+ memset(entry, 0, sizeof(*entry));
+ return HFSPLUS_INVALID_ATTR_RECORD;
+ }
}
+/*
+ * Find an attribute by its CNID and name in the attribute tree.
+ * This function searches for an attribute based on the CNID and name provided.
+ * If the name is NULL, it searches for the first attribute matching the CNID.
+ *
+ * sb: The superblock containing the filesystem.
+ * cnid: The CNID of the entry to search for.
+ * name: The name of the attribute to search for (optional).
+ * fd: The structure to store the search results.
+ *
+ * Returns: 0 on success, or an error code on failure.
+ */
int hfsplus_find_attr(struct super_block *sb, u32 cnid,
- const char *name, struct hfs_find_data *fd)
+ const char *name, struct hfs_find_data *fd)
{
int err = 0;
+ // Debugging output: log the search for the attribute.
hfs_dbg(ATTR_MOD, "find_attr: %s,%d\n", name ? name : NULL, cnid);
+ // If the attribute tree does not exist, log an error and return failure.
if (!HFSPLUS_SB(sb)->attr_tree) {
pr_err("attributes file doesn't exist\n");
return -EINVAL;
}
+ // Search by name if it is provided.
if (name) {
+ // Build the search key based on the CNID and the name.
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name);
if (err)
goto failed_find_attr;
+
+ // Perform the search by key in the attribute record tree.
err = hfs_brec_find(fd, hfs_find_rec_by_key);
if (err)
goto failed_find_attr;
} else {
+ // If no name is provided, search by CNID alone.
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL);
if (err)
goto failed_find_attr;
+
+ // Perform the search for the first record matching the CNID.
err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid);
if (err)
goto failed_find_attr;
}
-failed_find_attr:
- return err;
+ failed_find_attr:
+ return err; // Return the result of the attribute search (0 for success, error code on failure).
}
int hfsplus_attr_exists(struct inode *inode, const char *name)
diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
index 901e83d65d20..4d55e4c142ad 100644
--- a/fs/hfsplus/bfind.c
+++ b/fs/hfsplus/bfind.c
@@ -6,49 +6,70 @@
* Brad Boyer (flar@allandria.com)
* (C) 2003 Ardis Technologies <roman@ardistech.com>
*
- * Search routines for btrees
+ * Search routines for btrees in HFS Plus filesystem.
+ *
+ * Pranjal Prasad <prasadpranjal213@gmail.com>
+ * Improved and added comments
*/
#include <linux/slab.h>
#include "hfsplus_fs.h"
+// Initializes the find data structure, allocating memory for the search key.
int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
{
void *ptr;
+ // Initialize tree and bnode pointers
fd->tree = tree;
fd->bnode = NULL;
+
+ // Allocate memory for search key and associated structures.
ptr = kmalloc(tree->max_key_len * 2 + 4, GFP_KERNEL);
if (!ptr)
- return -ENOMEM;
- fd->search_key = ptr;
+ return -ENOMEM; // Return memory allocation error if failed.
+
+ // Set up pointers for the search key and key structure
+ fd->search_key = ptr;
fd->key = ptr + tree->max_key_len + 2;
+
+ // Log the initialization of the find operation
hfs_dbg(BNODE_REFS, "find_init: %d (%p)\n",
tree->cnid, __builtin_return_address(0));
- mutex_lock_nested(&tree->tree_lock,
- hfsplus_btree_lock_class(tree));
+
+ // Lock the btree structure to prevent concurrent modifications
+ mutex_lock_nested(&tree->tree_lock, hfsplus_btree_lock_class(tree));
+
return 0;
}
+// Frees resources allocated during the search operation.
void hfs_find_exit(struct hfs_find_data *fd)
{
+ // Release the bnode and free the allocated memory for search key.
hfs_bnode_put(fd->bnode);
kfree(fd->search_key);
+
+ // Log the exit from the find operation
hfs_dbg(BNODE_REFS, "find_exit: %d (%p)\n",
fd->tree->cnid, __builtin_return_address(0));
+
+ // Unlock the btree structure and set tree pointer to NULL
mutex_unlock(&fd->tree->tree_lock);
fd->tree = NULL;
}
+// Helper function to find the first record in a bnode matching the given CNID.
int hfs_find_1st_rec_by_cnid(struct hfs_bnode *bnode,
- struct hfs_find_data *fd,
- int *begin,
- int *end,
- int *cur_rec)
+ struct hfs_find_data *fd,
+ int *begin,
+ int *end,
+ int *cur_rec)
{
__be32 cur_cnid;
__be32 search_cnid;
+ // Check the CNID based on the btree type (ext, cat, or attr)
if (bnode->tree->cnid == HFSPLUS_EXT_CNID) {
cur_cnid = fd->key->ext.cnid;
search_cnid = fd->search_key->ext.cnid;
@@ -59,97 +80,107 @@ int hfs_find_1st_rec_by_cnid(struct hfs_bnode *bnode,
cur_cnid = fd->key->attr.cnid;
search_cnid = fd->search_key->attr.cnid;
} else {
- cur_cnid = 0; /* used-uninitialized warning */
+ cur_cnid = 0; // Used-uninitialized warning
search_cnid = 0;
- BUG();
+ BUG(); // Trigger a bug if CNID type is invalid.
}
+ // If the current CNID matches the search CNID, set the range and return.
if (cur_cnid == search_cnid) {
(*end) = (*cur_rec);
if ((*begin) == (*end))
- return 1;
+ return 1; // Found exact match.
} else {
+ // Adjust the range based on the comparison of CNIDs.
if (be32_to_cpu(cur_cnid) < be32_to_cpu(search_cnid))
(*begin) = (*cur_rec) + 1;
else
(*end) = (*cur_rec) - 1;
}
- return 0;
+ return 0; // Continue searching.
}
+// Helper function to compare keys and find the record matching the search key.
int hfs_find_rec_by_key(struct hfs_bnode *bnode,
- struct hfs_find_data *fd,
- int *begin,
- int *end,
- int *cur_rec)
+ struct hfs_find_data *fd,
+ int *begin,
+ int *end,
+ int *cur_rec)
{
int cmpval;
+ // Compare the keys using the btree's key comparison function.
cmpval = bnode->tree->keycmp(fd->key, fd->search_key);
if (!cmpval) {
(*end) = (*cur_rec);
- return 1;
+ return 1; // Found exact match.
}
+
+ // Adjust the search range based on the comparison result.
if (cmpval < 0)
(*begin) = (*cur_rec) + 1;
else
*(end) = (*cur_rec) - 1;
- return 0;
+ return 0; // Continue searching.
}
-/* Find the record in bnode that best matches key (not greater than...)*/
+// Function to find the best matching record in a bnode using binary search.
int __hfs_brec_find(struct hfs_bnode *bnode, struct hfs_find_data *fd,
- search_strategy_t rec_found)
+ search_strategy_t rec_found)
{
u16 off, len, keylen;
int rec;
int b, e;
int res;
- BUG_ON(!rec_found);
+ BUG_ON(!rec_found); // Ensure the strategy function is provided.
b = 0;
e = bnode->num_recs - 1;
- res = -ENOENT;
+ res = -ENOENT; // Default to record not found.
+
do {
- rec = (e + b) / 2;
+ rec = (e + b) / 2; // Binary search: Find the midpoint record.
len = hfs_brec_lenoff(bnode, rec, &off);
keylen = hfs_brec_keylen(bnode, rec);
if (keylen == 0) {
- res = -EINVAL;
+ res = -EINVAL; // Invalid record length.
goto fail;
}
hfs_bnode_read(bnode, fd->key, off, keylen);
+
+ // Check if the current record matches the search criteria.
if (rec_found(bnode, fd, &b, &e, &rec)) {
- res = 0;
+ res = 0; // Record found.
goto done;
}
- } while (b <= e);
+ } while (b <= e); // Continue binary search while the range is valid.
+ // If no exact match found, read the last record.
if (rec != e && e >= 0) {
len = hfs_brec_lenoff(bnode, e, &off);
keylen = hfs_brec_keylen(bnode, e);
if (keylen == 0) {
- res = -EINVAL;
+ res = -EINVAL; // Invalid record length.
goto fail;
}
hfs_bnode_read(bnode, fd->key, off, keylen);
}
-done:
+ done:
+ // Populate the find data structure with the found record details.
fd->record = e;
fd->keyoffset = off;
fd->keylength = keylen;
fd->entryoffset = off + keylen;
fd->entrylength = len - keylen;
-fail:
- return res;
+ fail:
+ return res; // Return the result (0 if found, error code otherwise).
}
-/* Traverse a B*Tree from the root to a leaf finding best fit to key */
-/* Return allocated copy of node found, set recnum to best record */
+// Traverse the B*Tree from the root to a leaf, finding the best match for the key.
int hfs_brec_find(struct hfs_find_data *fd, search_strategy_t do_key_compare)
{
struct hfs_btree *tree;
@@ -160,63 +191,77 @@ int hfs_brec_find(struct hfs_find_data *fd, search_strategy_t do_key_compare)
tree = fd->tree;
if (fd->bnode)
- hfs_bnode_put(fd->bnode);
- fd->bnode = NULL;
- nidx = tree->root;
+ hfs_bnode_put(fd->bnode); // Release any previous bnode.
+ fd->bnode = NULL;
+ nidx = tree->root; // Start with the root node.
+
if (!nidx)
- return -ENOENT;
- height = tree->depth;
+ return -ENOENT; // Return error if root node is invalid.
+
+ height = tree->depth;
res = 0;
parent = 0;
+
for (;;) {
+ // Find the bnode corresponding to the current index.
bnode = hfs_bnode_find(tree, nidx);
if (IS_ERR(bnode)) {
- res = PTR_ERR(bnode);
+ res = PTR_ERR(bnode); // Propagate error if node is invalid.
bnode = NULL;
break;
}
+
+ // Validate node height and type (should match the current depth and type).
if (bnode->height != height)
goto invalid;
if (bnode->type != (--height ? HFS_NODE_INDEX : HFS_NODE_LEAF))
goto invalid;
- bnode->parent = parent;
+ // Set the parent node index and perform the search within the node.
+ bnode->parent = parent;
res = __hfs_brec_find(bnode, fd, do_key_compare);
- if (!height)
+ if (!height) // Exit if we've reached the leaf level.
break;
- if (fd->record < 0)
+ if (fd->record < 0) // Handle invalid record.
goto release;
+ // Prepare for the next level of traversal.
parent = nidx;
hfs_bnode_read(bnode, &data, fd->entryoffset, 4);
nidx = be32_to_cpu(data);
hfs_bnode_put(bnode);
}
fd->bnode = bnode;
- return res;
+ return res; // Return the result of the search.
-invalid:
+ invalid:
pr_err("inconsistency in B*Tree (%d,%d,%d,%u,%u)\n",
- height, bnode->height, bnode->type, nidx, parent);
- res = -EIO;
-release:
- hfs_bnode_put(bnode);
+ height, bnode->height, bnode->type, nidx, parent);
+ res = -EIO; // Return I/O error for tree inconsistency.
+
+ release:
+ hfs_bnode_put(bnode); // Release the bnode.
return res;
}
+// Reads a record from the bnode based on the search data.
int hfs_brec_read(struct hfs_find_data *fd, void *rec, int rec_len)
{
int res;
+ // Find the record using the key comparison strategy.
res = hfs_brec_find(fd, hfs_find_rec_by_key);
if (res)
return res;
if (fd->entrylength > rec_len)
- return -EINVAL;
- hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
+ return -EINVAL; // Return error if record is larger than buffer.
+
+ // Read the record from the bnode into the provided buffer.
+ hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
return 0;
}
+// Goto a specific record (cnt) in the bnode, adjusting the position as needed.
int hfs_brec_goto(struct hfs_find_data *fd, int cnt)
{
struct hfs_btree *tree;
@@ -227,6 +272,7 @@ int hfs_brec_goto(struct hfs_find_data *fd, int cnt)
bnode = fd->bnode;
tree = bnode->tree;
+ // Move backwards if cnt is negative.
if (cnt < 0) {
cnt = -cnt;
while (cnt > fd->record) {
@@ -247,6 +293,7 @@ int hfs_brec_goto(struct hfs_find_data *fd, int cnt)
}
fd->record -= cnt;
} else {
+ // Move forwards if cnt is positive.
while (cnt >= bnode->num_recs - fd->record) {
cnt -= bnode->num_recs - fd->record;
fd->record = 0;
@@ -266,18 +313,22 @@ int hfs_brec_goto(struct hfs_find_data *fd, int cnt)
fd->record += cnt;
}
+ // Read the key and entry information for the target record.
len = hfs_brec_lenoff(bnode, fd->record, &off);
keylen = hfs_brec_keylen(bnode, fd->record);
if (keylen == 0) {
res = -EINVAL;
goto out;
}
+
+ // Update the find data with the record's information.
fd->keyoffset = off;
fd->keylength = keylen;
fd->entryoffset = off + keylen;
fd->entrylength = len - keylen;
hfs_bnode_read(bnode, fd->key, off, keylen);
-out:
+
+ out:
fd->bnode = bnode;
return res;
}
diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 9e1732a2b92a..9c0f4667606b 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -7,6 +7,8 @@
* (C) 2003 Ardis Technologies <roman@ardistech.com>
*
* Handle opening/closing btree
+ * Pranjal Prasad <prasadpranjal213@gmail.com>
+ * Improved and added comments
*/
#include <linux/slab.h>
@@ -18,15 +20,15 @@
/*
* Initial source code of clump size calculation is gotten
- * from http://opensource.apple.com/tarballs/diskdev_cmds/
+ * from https://github.com/apple-oss-distributions/diskdev_cmds/tags
*/
#define CLUMP_ENTRIES 15
static short clumptbl[CLUMP_ENTRIES * 3] = {
-/*
- * Volume Attributes Catalog Extents
- * Size Clump (MB) Clump (MB) Clump (MB)
- */
+ /*
+ * Volume Attributes Catalog Extents
+ * Size Clump (MB) Clump (MB) Clump (MB)
+ */
/* 1GB */ 4, 4, 4,
/* 2GB */ 6, 6, 4,
/* 4GB */ 8, 8, 4,
@@ -73,7 +75,7 @@ static short clumptbl[CLUMP_ENTRIES * 3] = {
};
u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size,
- u64 sectors, int file_id)
+ u64 sectors, int file_id)
{
u32 mod = max(node_size, block_size);
u32 clump_size;
@@ -82,15 +84,15 @@ u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size,
/* Figure out which column of the above table to use for this file. */
switch (file_id) {
- case HFSPLUS_ATTR_CNID:
- column = 0;
- break;
- case HFSPLUS_CAT_CNID:
- column = 1;
- break;
- default:
- column = 2;
- break;
+ case HFSPLUS_ATTR_CNID:
+ column = 0;
+ break;
+ case HFSPLUS_CAT_CNID:
+ column = 1;
+ break;
+ default:
+ column = 2;
+ break;
}
/*
@@ -105,7 +107,7 @@ u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size,
/* turn exponent into table index... */
for (i = 0, sectors = sectors >> 22;
sectors && (i < CLUMP_ENTRIES - 1);
- ++i, sectors = sectors >> 1) {
+ ++i, sectors = sectors >> 1) {
/* empty body */
}
@@ -164,7 +166,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
/* Load the header */
head = (struct hfs_btree_header_rec *)(kmap_local_page(page) +
- sizeof(struct hfs_bnode_desc));
+ sizeof(struct hfs_bnode_desc));
tree->root = be32_to_cpu(head->root);
tree->leaf_count = be32_to_cpu(head->leaf_count);
tree->leaf_head = be32_to_cpu(head->leaf_head);
@@ -176,84 +178,119 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
tree->max_key_len = be16_to_cpu(head->max_key_len);
tree->depth = be16_to_cpu(head->depth);
- /* Verify the tree and set the correct compare function */
+ /*
+ * Verify the B-tree structure and set the correct comparison function based on tree type (extent, catalog, or attributes).
+ * This section validates the B-tree properties such as the key length and flags, ensuring that they are consistent
+ * with the expected values for the respective B-tree type. It then sets the correct comparison function for key comparison.
+ */
switch (id) {
- case HFSPLUS_EXT_CNID:
- if (tree->max_key_len != HFSPLUS_EXT_KEYLEN - sizeof(u16)) {
- pr_err("invalid extent max_key_len %d\n",
- tree->max_key_len);
- goto fail_page;
- }
- if (tree->attributes & HFS_TREE_VARIDXKEYS) {
- pr_err("invalid extent btree flag\n");
- goto fail_page;
- }
+ case HFSPLUS_EXT_CNID:
+ /* For Extents B-tree:
+ * Ensure that the max key length is correct. The key length must match the expected length minus the size of a 16-bit value.
+ * If the key length is incorrect, log an error and fail the operation.
+ */
+ if (tree->max_key_len != HFSPLUS_EXT_KEYLEN - sizeof(u16)) {
+ pr_err("invalid extent max_key_len %d\n", tree->max_key_len);
+ goto fail_page; // Jump to failure handling if the key length is invalid.
+ }
- tree->keycmp = hfsplus_ext_cmp_key;
- break;
- case HFSPLUS_CAT_CNID:
- if (tree->max_key_len != HFSPLUS_CAT_KEYLEN - sizeof(u16)) {
- pr_err("invalid catalog max_key_len %d\n",
- tree->max_key_len);
- goto fail_page;
- }
- if (!(tree->attributes & HFS_TREE_VARIDXKEYS)) {
- pr_err("invalid catalog btree flag\n");
- goto fail_page;
- }
+ /* Check that the btree does not have the 'variable index keys' attribute, as it is not valid for Extents B-tree. */
+ if (tree->attributes & HFS_TREE_VARIDXKEYS) {
+ pr_err("invalid extent btree flag\n");
+ goto fail_page; // Jump to failure handling if the flag is invalid.
+ }
- if (test_bit(HFSPLUS_SB_HFSX, &HFSPLUS_SB(sb)->flags) &&
- (head->key_type == HFSPLUS_KEY_BINARY))
- tree->keycmp = hfsplus_cat_bin_cmp_key;
- else {
- tree->keycmp = hfsplus_cat_case_cmp_key;
- set_bit(HFSPLUS_SB_CASEFOLD, &HFSPLUS_SB(sb)->flags);
- }
- break;
- case HFSPLUS_ATTR_CNID:
- if (tree->max_key_len != HFSPLUS_ATTR_KEYLEN - sizeof(u16)) {
- pr_err("invalid attributes max_key_len %d\n",
- tree->max_key_len);
- goto fail_page;
- }
- tree->keycmp = hfsplus_attr_bin_cmp_key;
- break;
- default:
- pr_err("unknown B*Tree requested\n");
- goto fail_page;
+ /* Set the appropriate comparison function for extent keys. */
+ tree->keycmp = hfsplus_ext_cmp_key;
+ break;
+
+ case HFSPLUS_CAT_CNID:
+ /* For Catalog B-tree:
+ * Ensure the max key length is correct. Catalog keys must match a fixed length (HFSPLUS_CAT_KEYLEN - sizeof(u16)).
+ */
+ if (tree->max_key_len != HFSPLUS_CAT_KEYLEN - sizeof(u16)) {
+ pr_err("invalid catalog max_key_len %d\n", tree->max_key_len);
+ goto fail_page; // Jump to failure handling if the key length is invalid.
+ }
+
+ /* Ensure that the Catalog B-tree has the 'variable index keys' flag set. This is required for Catalogs. */
+ if (!(tree->attributes & HFS_TREE_VARIDXKEYS)) {
+ pr_err("invalid catalog btree flag\n");
+ goto fail_page; // Jump to failure handling if the flag is missing.
+ }
+
+ /* If the HFSX flag is set and the key type is binary, use the binary key comparison function. Otherwise, use case-insensitive comparison. */
+ if (test_bit(HFSPLUS_SB_HFSX, &HFSPLUS_SB(sb)->flags) &&
+ (head->key_type == HFSPLUS_KEY_BINARY)) {
+ tree->keycmp = hfsplus_cat_bin_cmp_key; // Binary key comparison for HFSX volumes.
+ } else {
+ /* For non-HFSX or case-sensitive volumes, use case-insensitive comparison. */
+ tree->keycmp = hfsplus_cat_case_cmp_key;
+ set_bit(HFSPLUS_SB_CASEFOLD, &HFSPLUS_SB(sb)->flags); // Set flag for case-insensitive comparison.
+ }
+ break;
+
+ case HFSPLUS_ATTR_CNID:
+ /* For Attributes B-tree:
+ * Ensure that the key length matches the expected value (HFSPLUS_ATTR_KEYLEN - sizeof(u16)).
+ */
+ if (tree->max_key_len != HFSPLUS_ATTR_KEYLEN - sizeof(u16)) {
+ pr_err("invalid attributes max_key_len %d\n", tree->max_key_len);
+ goto fail_page; // Jump to failure handling if the key length is invalid.
+ }
+
+ /* Set the appropriate comparison function for attribute keys (binary comparison). */
+ tree->keycmp = hfsplus_attr_bin_cmp_key;
+ break;
+
+ default:
+ /* If the B-tree ID does not match any known types, log an error and fail. */
+ pr_err("unknown B*Tree requested\n");
+ goto fail_page; // Jump to failure handling for unknown B-tree types.
}
+ /*
+ * Perform final validation and setup for the B-tree:
+ * - Ensure the tree has the 'big keys' attribute set (required for proper B-tree functionality).
+ * - Check that the node size is a valid power of 2.
+ * - Ensure that the B-tree has a non-zero node count.
+ */
if (!(tree->attributes & HFS_TREE_BIGKEYS)) {
pr_err("invalid btree flag\n");
- goto fail_page;
+ goto fail_page; // Fail if the 'big keys' flag is missing.
}
+ /* Ensure the node size is a valid power of 2. If not, it's an invalid configuration. */
size = tree->node_size;
if (!is_power_of_2(size))
- goto fail_page;
- if (!tree->node_count)
- goto fail_page;
+ goto fail_page; // Jump to failure handling if node size is not a power of 2.
- tree->node_size_shift = ffs(size) - 1;
+ /* Ensure that the node count is greater than zero. */
+ if (!tree->node_count)
+ goto fail_page; // Jump to failure handling if node count is zero.
- tree->pages_per_bnode =
- (tree->node_size + PAGE_SIZE - 1) >>
- PAGE_SHIFT;
+ tree->node_size_shift = ffs(size) - 1; // Calculate the node size shift based on the node size.
- kunmap_local(head);
- put_page(page);
- return tree;
+ tree->pages_per_bnode = (tree->node_size + PAGE_SIZE - 1) >> PAGE_SHIFT; // Calculate pages per B-node.
- fail_page:
- kunmap_local(head);
- put_page(page);
- free_inode:
- tree->inode->i_mapping->a_ops = &hfsplus_aops;
- iput(tree->inode);
- free_tree:
- kfree(tree);
- return NULL;
-}
+ kunmap_local(head); // Unmap the page after processing.
+ put_page(page); // Release the page.
+
+ return tree; // Return the successfully opened B-tree.
+
+ /* Failure handling section: Clean up resources in case of an error. */
+ fail_page:
+ kunmap_local(head); // Ensure we unmap the page in case of failure.
+ put_page(page); // Release the page.
+
+ free_inode:
+ tree->inode->i_mapping->a_ops = &hfsplus_aops; // Restore the original address space operations.
+ iput(tree->inode); // Release the inode associated with this B-tree.
+
+ free_tree:
+ kfree(tree); // Free the B-tree structure.
+
+ return NULL; // Return NULL to indicate failure.
/* Release resources used by a btree */
void hfs_btree_close(struct hfs_btree *tree)
@@ -269,11 +306,11 @@ void hfs_btree_close(struct hfs_btree *tree)
tree->node_hash[i] = node->next_hash;
if (atomic_read(&node->refcnt))
pr_crit("node %d:%d "
- "still has %d user(s)!\n",
+ "still has %d user(s)!\n",
node->tree->cnid, node->this,
- atomic_read(&node->refcnt));
- hfs_bnode_free(node);
- tree->node_hash_cnt--;
+ atomic_read(&node->refcnt));
+ hfs_bnode_free(node);
+ tree->node_hash_cnt--;
}
}
iput(tree->inode);
@@ -293,7 +330,7 @@ int hfs_btree_write(struct hfs_btree *tree)
/* Load the header */
page = node->page[0];
head = (struct hfs_btree_header_rec *)(kmap_local_page(page) +
- sizeof(struct hfs_bnode_desc));
+ sizeof(struct hfs_bnode_desc));
head->root = cpu_to_be32(tree->root);
head->leaf_count = cpu_to_be32(tree->leaf_count);
@@ -359,10 +396,10 @@ int hfs_bmap_reserve(struct hfs_btree *tree, int rsvd_nodes)
if (res)
return res;
hip->phys_size = inode->i_size =
- (loff_t)hip->alloc_blocks <<
- HFSPLUS_SB(tree->sb)->alloc_blksz_shift;
+ (loff_t)hip->alloc_blocks <<
+ HFSPLUS_SB(tree->sb)->alloc_blksz_shift;
hip->fs_blocks =
- hip->alloc_blocks << HFSPLUS_SB(tree->sb)->fs_shift;
+ hip->alloc_blocks << HFSPLUS_SB(tree->sb)->fs_shift;
inode_set_bytes(inode, inode->i_size);
count = inode->i_size >> tree->node_size_shift;
tree->free_nodes += count - tree->node_count;
@@ -413,7 +450,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
mark_inode_dirty(tree->inode);
hfs_bnode_put(node);
return hfs_bnode_create(tree,
- idx);
+ idx);
}
}
}
@@ -470,8 +507,8 @@ void hfs_bmap_free(struct hfs_bnode *node)
if (!i) {
/* panic */;
pr_crit("unable to free bnode %u. "
- "bmap not found!\n",
- node->this);
+ "bmap not found!\n",
+ node->this);
hfs_bnode_put(node);
return;
}
@@ -482,7 +519,7 @@ void hfs_bmap_free(struct hfs_bnode *node)
if (node->type != HFS_NODE_MAP) {
/* panic */;
pr_crit("invalid bmap found! "
- "(%u,%d)\n",
+ "(%u,%d)\n",
node->this, node->type);
hfs_bnode_put(node);
return;
@@ -497,7 +534,7 @@ void hfs_bmap_free(struct hfs_bnode *node)
byte = data[off];
if (!(byte & m)) {
pr_crit("trying to free free bnode "
- "%u(%d)\n",
+ "%u(%d)\n",
node->this, node->type);
kunmap_local(data);
hfs_bnode_put(node);
diff --git a/fs/hfsplus/xattr_security.c b/fs/hfsplus/xattr_security.c
index 90f68ec119cd..db39a8632b2c 100644
--- a/fs/hfsplus/xattr_security.c
+++ b/fs/hfsplus/xattr_security.c
@@ -17,9 +17,13 @@ static int hfsplus_security_getxattr(const struct xattr_handler *handler,
struct dentry *unused, struct inode *inode,
const char *name, void *buffer, size_t size)
{
+ /* Ensure valid parameters */
+ if (!name || !buffer)
+ return -EINVAL;
+
return hfsplus_getxattr(inode, name, buffer, size,
XATTR_SECURITY_PREFIX,
- XATTR_SECURITY_PREFIX_LEN);
+ XATTR_SECURITY_PREFIX_LEN);
}
static int hfsplus_security_setxattr(const struct xattr_handler *handler,
@@ -28,48 +32,62 @@ static int hfsplus_security_setxattr(const struct xattr_handler *handler,
const char *name, const void *buffer,
size_t size, int flags)
{
+ /* Ensure valid parameters */
+ if (!name || !buffer || size == 0)
+ return -EINVAL;
+
return hfsplus_setxattr(inode, name, buffer, size, flags,
XATTR_SECURITY_PREFIX,
- XATTR_SECURITY_PREFIX_LEN);
+ XATTR_SECURITY_PREFIX_LEN);
}
static int hfsplus_initxattrs(struct inode *inode,
- const struct xattr *xattr_array,
- void *fs_info)
+ const struct xattr *xattr_array,
+ void *fs_info)
{
const struct xattr *xattr;
char *xattr_name;
int err = 0;
- xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
- GFP_KERNEL);
+ /* Allocate buffer for xattr name */
+ xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, GFP_KERNEL);
if (!xattr_name)
return -ENOMEM;
- for (xattr = xattr_array; xattr->name != NULL; xattr++) {
- if (!strcmp(xattr->name, ""))
+ /* Iterate over the provided xattrs */
+ for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+ /* Skip if xattr name is empty or NULL */
+ if (!xattr->name || strcmp(xattr->name, "") == 0)
continue;
- strcpy(xattr_name, XATTR_SECURITY_PREFIX);
- strcpy(xattr_name +
- XATTR_SECURITY_PREFIX_LEN, xattr->name);
- memset(xattr_name +
- XATTR_SECURITY_PREFIX_LEN + strlen(xattr->name), 0, 1);
+ /* Copy the security prefix and xattr name into xattr_name */
+ strncpy(xattr_name, XATTR_SECURITY_PREFIX, NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN);
+ strncpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, xattr->name, NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN);
+ /* Ensure null-termination of the name */
+ xattr_name[XATTR_SECURITY_PREFIX_LEN + strlen(xattr->name)] = '\0';
+
+ /* Set the xattr on the inode */
err = __hfsplus_setxattr(inode, xattr_name,
- xattr->value, xattr->value_len, 0);
+ xattr->value, xattr->value_len, 0);
if (err)
break;
}
+
+ /* Free allocated memory */
kfree(xattr_name);
return err;
}
int hfsplus_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr)
+ const struct qstr *qstr)
{
+ /* Ensure valid parameters */
+ if (!inode || !dir || !qstr)
+ return -EINVAL;
+
return security_inode_init_security(inode, dir, qstr,
- &hfsplus_initxattrs, NULL);
+ &hfsplus_initxattrs, NULL);
}
const struct xattr_handler hfsplus_xattr_security_handler = {
--
2.48.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] Improved hfsplus in the kernel
2025-02-25 20:30 [PATCH] Improved hfsplus in the kernel Pranjal Prasad
@ 2025-02-25 20:59 ` Al Viro
0 siblings, 0 replies; 2+ messages in thread
From: Al Viro @ 2025-02-25 20:59 UTC (permalink / raw)
To: Pranjal Prasad; +Cc: linux-kernel
On Wed, Feb 26, 2025 at 02:00:36AM +0530, Pranjal Prasad wrote:
> Hi,
>
> Please find attached the patch to improve the HFS+ filesystem support in the
> kernel. I have not done much work, but as I require HFS, HFS+, and APFS in
> Linux, I decided to maintain it. Please allow me to be the maintainer of HFS
> and HFS+ in the kernel.
Umm... "Improved" is not a description - it's marketdroidese.
And comments are supposed to help understanding why we are doing
something, not retelling what's being done in every line.
Being more verbose is no virtue - not when it adds no information.
What's more worrying, though... are there actual changes in
that pile of added comments? Your "improved and added comments"
would seem to imply that these had been two distinct things,
so it sounds like there are actual modifications involved.
In such case you really should separate those rather than
burying them.
And this "every line must come with a comment, no matter how useful it is"
is a habit best unlearnt. I realize that a bunch of places teach that,
but it's a really, really bad idea.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-02-25 20:59 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-25 20:30 [PATCH] Improved hfsplus in the kernel Pranjal Prasad
2025-02-25 20:59 ` Al Viro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.