fs: Enhance read-only filesystem error notification mechanism #1268
Conversation
Reviewer's GuideRefactors Deepin filesystem error notification from a procfs/sysctl and overlayfs-specific hook into a generic, richer Generic Netlink-based facility, centralizes EROFS notification logic with a helper macro and path helper struct, and extends call sites to report more detailed context including multiple paths for rename failures. Class diagram for Deepin filesystem error notification data structures and helpersclassDiagram
class deepin_path_last {
+struct_path path
+const_char* last
}
class deepin_fs_err_event_data {
+const_char** filenames
+int count
+const_ino_t* inodes
+const_bool* inode_is_parent
+pid_t pid
+pid_t ppid
+const_char* comm
+uid_t uid
+gid_t gid
}
class deepin_err_notify_core {
+bool deepin_err_notify_initialized
+int deepin_err_notify_enable
+int deepin_err_notify_enabled()
+int deepin_err_notify_should_send()
+int deepin_lookup_path_or_parent(int dfd, struct_filename* name, unsigned_int flags, struct_deepin_path_last* result_path_last)
+void deepin_put_path_last(struct_deepin_path_last* path_last)
+void deepin_check_and_notify_ro_fs_err(const_struct_deepin_path_last* path_last, const_char* func_name)
+void deepin_notify_rename_ro_fs_err(const_struct_qstr* old_last, const_struct_qstr* new_last, const_struct_path* old_path, const_struct_path* new_path)
+int notify_fs_error(const_struct_deepin_fs_err_event_data* event)
+int prepare_and_notify_fs_error(const_struct_deepin_path_last* path_lasts, int path_last_count)
}
class deepin_err_notify_netlink {
+char[] DEEPIN_ERR_NOTIFY_FAMILY_NAME
+char[] MULTICAST_GROUP_ERR_EVENTS
+enum DEEPIN_ERR_NOTIFY_ATTR_*
+enum DEEPIN_ERR_NOTIFY_CMD_*
+struct_genl_family deepin_err_notify_genl_family
+const_struct_genl_multicast_group[] deepin_err_notify_nl_mcgrps
+const_struct_nla_policy[] deepin_err_notify_genl_policy
+const_struct_genl_ops[] deepin_err_notify_genl_ops
+int deepin_err_notify_cmd_enable(struct_sk_buff* skb, struct_genl_info* info)
+int deepin_err_notify_cmd_disable(struct_sk_buff* skb, struct_genl_info* info)
+int deepin_err_notify_cmd_get_status(struct_sk_buff* skb, struct_genl_info* info)
}
deepin_err_notify_core --> deepin_path_last : uses
deepin_err_notify_core --> deepin_fs_err_event_data : builds_and_sends
deepin_err_notify_core --> deepin_err_notify_netlink : sends_multicast
deepin_err_notify_netlink --> deepin_err_notify_core : updates_enable_flag
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @electricface. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Deepin error notification system from a direct netlink-based implementation to a notifier chain pattern, enabling external DKMS modules to register for filesystem error notifications. The callback signature in super_operations is changed from a boolean check (deepin_should_notify_error) to a notification handler (deepin_notify_error).
Key changes:
- Replaces the
deepin_should_notify_errorboolean callback with adeepin_notify_errorcallback that handles notifications directly - Implements a blocking notifier chain in overlayfs to allow external modules to subscribe to error events
- Refactors
do_renameat2to pass both old and new paths for rename operations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| include/linux/fs.h | Updates super_operations to change callback from boolean check to notification handler with multiple filenames |
| fs/overlayfs/super.c | Implements new notification callback, integrates notifier chain module, adds initialization logging |
| fs/overlayfs/deepin_ovl_err_notify.h | New header defining notifier chain API, error data structures, and exported functions |
| fs/overlayfs/deepin_ovl_err_notify.c | New implementation of blocking notifier chain with registration/unregistration/notification functions |
| fs/overlayfs/Makefile | Conditionally compiles the new notifier module when CONFIG_DEEPIN_ERR_NOTIFY is enabled |
| fs/namei.c | Refactors rename error handling to extract both paths and use new dual-path notification function |
| fs/internal.h | Adds declaration for new dual-path notification function |
| fs/deepin_err_notify.c | Removes netlink implementation, updates callbacks to use new filesystem notification API, adds dual-path handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fs/internal.h
Outdated
| int deepin_err_notify_enabled(void); | ||
| void deepin_check_and_notify_ro_fs_err(const struct path *path, const char *func_name); | ||
| void deepin_check_and_notify_ro_fs_err_paths(const struct path *path, const struct path *path_new, const char *func_name); | ||
| void deepin_send_ro_fs_err_notification(const char *filename, const char *func_name); |
There was a problem hiding this comment.
The function deepin_send_ro_fs_err_notification is declared in the header but no longer exists in the implementation (fs/deepin_err_notify.c). This unused declaration should be removed.
| void deepin_send_ro_fs_err_notification(const char *filename, const char *func_name); |
fs/deepin_err_notify.c
Outdated
| for (i = 0; i < 2; i++) { | ||
| if (!paths[i] || !paths[i]->mnt || !paths[i]->mnt->mnt_sb) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
Corrected whitespace: trailing whitespace on line 126 should be removed.
fs/deepin_err_notify.c
Outdated
| genlmsg_multicast(&deepin_err_notify_genl_family, skb, 0, 0, GFP_NOFS); | ||
| return; | ||
| /* Call the callback with collected filenames */ | ||
| if (filename_count == 2) { |
There was a problem hiding this comment.
The notification is only sent if exactly 2 filenames are collected. If only one path is valid (filename_count == 1), no notification is sent at all. This silently drops valid error notifications when only one of the two paths can be resolved. Consider calling the notification with whatever filenames were successfully collected (if filename_count > 0).
| if (filename_count == 2) { | |
| if (filename_count > 0) { |
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
63ef62c to
27bcd26
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two-path scenario in rename operations. - Introduced support for notifying read-only filesystem errors during rename operations. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
27bcd26 to
c1a8a11
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
c1a8a11 to
e590fa6
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
e590fa6 to
4f7e333
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:67` </location>
<code_context>
- },
-};
+/* Check multiple paths and send read-only error notification */
+void deepin_check_and_notify_ro_fs_err_paths(const struct path *path,
+ const struct path *path_new,
+ const char *func_name)
</code_context>
<issue_to_address>
**issue (complexity):** Consider merging the two-pass logic into a single loop with one buffer allocation for both paths to simplify the function.
Consider merging the two passes and doing a single allocation for both path buffers. For example, you can:
• Allocate one contiguous buffer of size PATH_MAX * 2
• Scan both paths in one loop:
– On first valid entry grab `sb->s_op` and bail out if no callback
– On every valid entry do `d_path()` into your slice of the buffer
– Keep a simple array of pointers into that buffer
• Call `deepin_notify_error()` once with the collected filenames
This removes the two‐pass logic, duplicates, and per‐path kmalloc()/kfree() calls:
```c
void deepin_check_and_notify_ro_fs_err_paths(
const struct path *path1,
const struct path *path2,
const char *func_name)
{
const struct path *paths[2] = { path1, path2 };
const char *fnames[2];
int count = 0;
struct super_block *sb = NULL;
const struct super_operations *op = NULL;
char *buf;
/* single kmalloc for both buffers */
buf = kmalloc(PATH_MAX * 2, GFP_KERNEL);
if (!buf)
return;
if (!__ratelimit(&deepin_ro_fs_err_ratelimit))
goto out;
for (int i = 0; i < 2; i++) {
const struct path *p = paths[i];
char *dst = buf + i * PATH_MAX;
char *res;
if (!p || !p->mnt || !p->mnt->mnt_sb || !p->dentry)
continue;
/* grab callback once */
if (!sb) {
sb = p->mnt->mnt_sb;
op = sb->s_op;
if (!op || !op->deepin_notify_error)
goto out;
} else if (p->mnt->mnt_sb != sb) {
continue;
}
res = d_path(p, dst, PATH_MAX);
if (IS_ERR(res))
continue;
fnames[count++] = res;
}
if (count)
op->deepin_notify_error(sb, fnames, count, func_name);
out:
kfree(buf);
}
```
This keeps all original semantics, reduces nested loops, duplicates and per-path kmalloc/free, and should be easier to follow.
</issue_to_address>
### Comment 2
<location> `fs/namei.c:4297` </location>
<code_context>
inode_unlock(path.dentry->d_inode);
mnt_drop_write(path.mnt);
exit2:
-#ifdef CONFIG_DEEPIN_ERR_NOTIFY
- if (unlikely((error == -EROFS) && deepin_err_notify_enabled())) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving the rename error notification logic into a Deepin module helper to simplify namei.c.
```suggestion
Instead of embedding a 50-line `notify_rename_ro_fs_err()` in namei.c, move it into your Deepin module and expose a single helper. In namei.c you’ll end up with just:
```c
exit2:
if (deepin_should_notify_ro_fs_err(error))
deepin_notify_rename_ro_fs_err(
old_path.mnt, old_path.dentry,
new_path.mnt, new_path.dentry,
&old_last, &new_last);
/* … */
```
Then in fs/deepin/err_notify.c (or wherever your notify helper lives), define:
```c
void deepin_notify_rename_ro_fs_err(
struct vfsmount *old_mnt,
struct dentry *old_parent,
struct vfsmount *new_mnt,
struct dentry *new_parent,
const struct qstr *old_last,
const struct qstr *new_last)
{
struct path old_file = { .mnt = NULL, .dentry = NULL };
struct path new_file = { .mnt = NULL, .dentry = NULL };
struct dentry *d;
d = lookup_one_qstr_excl(old_last, old_parent, 0);
if (!IS_ERR(d) && d_is_positive(d))
old_file = (struct path){ .mnt = old_mnt, .dentry = d };
else if (!IS_ERR(d))
dput(d);
d = lookup_one_qstr_excl(new_last, new_parent, 0);
if (!IS_ERR(d) && d_is_positive(d))
new_file = (struct path){ .mnt = new_mnt, .dentry = d };
else if (!IS_ERR(d))
dput(d);
if (old_file.dentry && new_file.dentry)
deepin_check_and_notify_ro_fs_err_paths(
&old_file, &new_file, "rename");
dput(old_file.dentry);
dput(new_file.dentry);
}
```
This pulls all the lookup/teardown out of namei.c, keeps the callsite minimal, and reuses your existing notification infrastructure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
4f7e333 to
dc62cbd
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
dc62cbd to
2dcfd68
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
2 times, most recently
from
894fa4d to
04001ca
Compare
The error notification mechanism has been rewritten: instead of sending netlink messages directly from the fs module, it now uses a Blocking Notifier Chain within the overlayfs module, enabling external modules to send netlink messages later. Key changes: - Replaced `deepin_should_notify_error` with `deepin_notify_error` in `super_operations`. - Added handling for the two paths that need to be reported in the rename system call. - Added a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
|
/ok-to-test |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The timestamp attribute is documented as “nanoseconds since boot” but you use ktime_get_real_ns() (nanoseconds since epoch); please update the comment or switch to ktime_get_ns() for monotonic timing.
- The Generic Netlink family is registered in init but never unregistered; if this code can be built as a module, add a corresponding genl_unregister_family in your exit/cleanup path to avoid resource leaks.
- All the new netlink logic is unconditionally compiled—consider wrapping the includes and code under CONFIG_NET/CONFIG_GENERIC_NETLINK to ensure it still builds correctly when netlink support is disabled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timestamp attribute is documented as “nanoseconds since boot” but you use ktime_get_real_ns() (nanoseconds since epoch); please update the comment or switch to ktime_get_ns() for monotonic timing.
- The Generic Netlink family is registered in init but never unregistered; if this code can be built as a module, add a corresponding genl_unregister_family in your exit/cleanup path to avoid resource leaks.
- All the new netlink logic is unconditionally compiled—consider wrapping the includes and code under CONFIG_NET/CONFIG_GENERIC_NETLINK to ensure it still builds correctly when netlink support is disabled.
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:463` </location>
<code_context>
+ *
+ * Returns: 0 on success, negative error code on failure
+ */
+static int notify_fs_error(const struct deepin_fs_err_event_data *event)
+{
+ int msg_size = 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** No check for message size exceeding netlink limits.
The current implementation may fail if the message size exceeds netlink limits due to large or numerous attributes. Please add checks to enforce these limits, such as restricting the number of filenames or truncating attributes.
</issue_to_address>
### Comment 2
<location> `fs/deepin_err_notify.c:661` </location>
<code_context>
+ *
+ * Returns: 0 on success, negative error code on failure
+ */
+static int prepare_and_notify_fs_error(const struct path **paths,
+ int path_count, const char *func_name)
+{
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the file by extracting process info helpers, Netlink attribute utilities, and splitting large functions into smaller units for better maintainability.
Here are a few concrete refactorings that can dramatically trim this file down, without changing any behaviour:
1) Extract “process‐info” helpers into their own unit:
• Move `get_process_cmdline()`, `get_process_exe()`, `get_process_cgroup()` into a new `proc_info.c` + `proc_info.h`.
• Change calls in your big file to:
```c
#include "proc_info.h"
…
cmdline_len = proc_info_get_cmdline(current, proc_cmdline, PROC_CMDLINE_BUF_SIZE);
```
2) Pull out Netlink attribute sizing/put loops into small helpers in e.g. `nl_util.{c,h}`:
nl_util.h
```c
size_t nl_size_string(const char *s)
{
return s ? nla_total_size(strlen(s)+1) : 0;
}
static inline int nl_put_str(struct sk_buff *skb, int attr, const char *s)
{
return s ? nla_put_string(skb, attr, s) : 0;
}
static inline int nl_put_pair_u64(struct sk_buff *skb, int attr, u64 v)
{
return nla_put_u64_64bit(skb, attr, v, 0);
}
```
Then your size‐calc & put loops collapse to:
```c
/* calculate size */
msg_size = nl_total_size_pair_u32() /* pid */
+ nl_size_string(event->cmdline)
+ …;
for (i=0; i<event->filename_count; i++)
msg_size += nl_size_string(event->filenames[i])
+ nla_total_size(sizeof(u64))
+ nla_total_size(sizeof(u8));
/* building message */
error = nl_put_str(skb, DEEPIN_ERR_NOTIFY_ATTR_CMD_LINE, event->cmdline);
error |= nl_put_str(skb, DEEPIN_ERR_NOTIFY_ATTR_COMM, event->comm);
…
for (i=0; i<event->filename_count; i++) {
error |= nl_put_str(skb, DEEPIN_ERR_NOTIFY_ATTR_FILENAME,
event->filenames[i]);
error |= nl_put_pair_u64(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE,
event->inodes[i]);
error |= nla_put_u8(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT,
event->inode_is_parent[i]);
if (error) goto attr_err_out;
}
```
3) Break the huge `prepare_and_notify_fs_error()` into three pieces in the same file:
```c
static int collect_paths(…);
static int collect_proc_info(…);
static int build_and_multicast(…);
```
Then your top‐level becomes:
```c
static int prepare_and_notify_fs_error(const struct path **paths, int path_count,
const char *func_name)
{
struct deepin_fs_err_event_data event = { 0 };
int err;
err = collect_paths(&event, paths, path_count);
if (err) return err;
err = collect_proc_info(&event, func_name);
if (err) goto free_paths;
err = build_and_multicast(&event);
free_paths:
free_event_data(&event);
return err;
}
```
By moving each conceptual chunk into its own file or at least into its own small function, you get much smaller, more focused units—easier to read, test, and maintain—while preserving 100% of the new functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Added handling for the two paths that need to be reported in the rename system call. - Introduced a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. - Collect more detailed information when an error occurs. - Replaced the `/proc/sys/fs/deepin-err-notify-enable` file with multiple netlink commands. - Reverted modifications to overlayfs; the `deepin_err_notify` mount option is no longer available. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
cb4931a to
90348ec
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int deepin_err_notify_enabled(void); | ||
| void deepin_check_and_notify_ro_fs_err(const struct path *path, const char *func_name); | ||
| void deepin_send_ro_fs_err_notification(const char *filename, const char *func_name); | ||
| void deepin_check_and_notify_ro_fs_err(const struct path *path, | ||
| const char *func_name); | ||
| void deepin_check_and_notify_ro_fs_err_paths(const struct path *path, | ||
| const struct path *path_new, | ||
| const char *func_name); | ||
| void deepin_notify_rename_ro_fs_err(const struct qstr *old_last, | ||
| const struct qstr *new_last, | ||
| const struct path *old_path, | ||
| const struct path *new_path); |
There was a problem hiding this comment.
Missing stub implementations for deepin error notification functions when CONFIG_DEEPIN_ERR_NOTIFY is not defined. Since fs/deepin_err_notify.c is only compiled when CONFIG_DEEPIN_ERR_NOTIFY is set (per fs/Makefile), these function declarations will cause undefined symbol errors at link time when the config option is disabled. Add inline stub implementations inside an #else block paired with the existing #ifdef CONFIG_DEEPIN_ERR_NOTIFY at line 81.
fs/deepin_err_notify.c
Outdated
| /* Early return if paths are invalid */ | ||
| if ((!path || !path->mnt || !path->mnt->mnt_sb || !path->dentry) && |
There was a problem hiding this comment.
The validation logic uses AND (&&) when it should use OR (||) between the two path checks. The current condition only returns early when BOTH paths are invalid, which means the function proceeds even when only one path is valid. This can cause null pointer dereferences in prepare_and_notify_fs_error. Change && to || to ensure the function returns early unless at least one valid path exists.
| /* Early return if paths are invalid */ | |
| if ((!path || !path->mnt || !path->mnt->mnt_sb || !path->dentry) && | |
| /* Early return if both paths are invalid */ | |
| if ((!path || !path->mnt || !path->mnt->mnt_sb || !path->dentry) || |
fs/deepin_err_notify.c
Outdated
| void deepin_check_and_notify_ro_fs_err(const struct path *path, | ||
| const char *func_name) |
There was a problem hiding this comment.
The func_name parameter is declared but never used in this function. This parameter should either be removed or passed through to prepare_and_notify_fs_error if it's intended for logging purposes. The same issue exists in deepin_check_and_notify_ro_fs_err_paths.
fs/deepin_err_notify.c
Outdated
| new_dentry_tmp = lookup_one_qstr_excl(new_last, new_path->dentry, 0); | ||
| if (!IS_ERR(new_dentry_tmp)) { | ||
| new_file_path.mnt = new_path->mnt; | ||
| new_file_path.dentry = new_dentry_tmp; | ||
| } | ||
|
|
||
| /* Call notification with both paths only if both are valid */ | ||
| if (old_file_path.dentry && new_file_path.dentry) { | ||
| deepin_check_and_notify_ro_fs_err_paths( | ||
| &old_file_path, &new_file_path, "rename"); | ||
| } | ||
|
|
||
| if (!IS_ERR(old_dentry_tmp)) | ||
| dput(old_dentry_tmp); | ||
| if (!IS_ERR(new_dentry_tmp)) | ||
| dput(new_dentry_tmp); |
There was a problem hiding this comment.
Potential dentry reference leak when only one path is successfully looked up. If old_dentry_tmp lookup succeeds but new_dentry_tmp lookup fails (or vice versa), the function still holds a reference from the successful lookup but exits at line 618 without calling the notification. The dput() calls at lines 620-623 will properly release the dentries, but this creates an inconsistent behavior where some EROFS errors during rename are silently ignored. Consider either: (1) calling notification with the single valid path, or (2) adding a comment explaining why this case is intentionally not notified.
fs/deepin_err_notify.c
Outdated
| /* Convert each path to a filename string and extract inode */ | ||
| for (i = 0; i < path_count; i++) { | ||
| if (paths[i] && paths[i]->dentry) { | ||
| struct dentry *target_dentry; | ||
| bool use_parent; | ||
|
|
||
| path_bufs[i] = kmalloc(PATH_MAX, GFP_ATOMIC); | ||
| if (path_bufs[i]) { | ||
| char *p = d_path(paths[i], path_bufs[i], | ||
| PATH_MAX); | ||
| if (!IS_ERR(p)) { | ||
| filenames[filename_count++] = p; | ||
| } else { | ||
| pr_debug( | ||
| "deepin_err_notify: d_path failed for path[%d]: %ld\n", | ||
| i, PTR_ERR(p)); | ||
| } | ||
| } else { | ||
| pr_warn("deepin_err_notify: Failed to allocate path buffer[%d]\n", | ||
| i); | ||
| } | ||
|
|
||
| /* Extract inode: prefer current dentry, fallback to parent */ | ||
| target_dentry = paths[i]->dentry; | ||
| use_parent = false; | ||
|
|
||
| if (!target_dentry->d_inode && target_dentry->d_parent) { | ||
| /* | ||
| * If current dentry has no inode (negative dentry), | ||
| * try to use parent dentry's inode | ||
| */ | ||
| target_dentry = target_dentry->d_parent; | ||
| use_parent = true; | ||
| } | ||
|
|
||
| if (target_dentry->d_inode) { | ||
| inodes[i] = target_dentry->d_inode->i_ino; | ||
| inode_parent_flags[i] = use_parent; | ||
| } else { | ||
| /* No inode available even from parent */ | ||
| inodes[i] = 0; | ||
| inode_parent_flags[i] = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Array indexing mismatch between filenames and inodes arrays. The filenames array is indexed by filename_count (which is incremented at line 463), but the inodes and inode_parent_flags arrays are indexed by i. When d_path() fails for paths[0] but succeeds for paths[1], filename_count will be 0 when processing paths[1], causing filenames[0] to be set from paths[1], while inodes[1] gets the inode from paths[1]. This creates a mismatch where filenames[j] does not correspond to inodes[j]. Fix this by using a consistent index for all three arrays, or track the path index separately in the filenames array.
fs/deepin_err_notify.c
Outdated
| void deepin_check_and_notify_ro_fs_err_paths(const struct path *path, | ||
| const struct path *path_new, | ||
| const char *func_name) |
There was a problem hiding this comment.
The func_name parameter is declared but never used in this function. Consider removing it or passing it to prepare_and_notify_fs_error if logging the operation name is intended.
- Added handling for the two paths that need to be reported in the rename system call. - Introduced a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. - Collect more detailed information when an error occurs. - Replaced the `/proc/sys/fs/deepin-err-notify-enable` file with multiple netlink commands. - Reverted modifications to overlayfs; the `deepin_err_notify` mount option is no longer available. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
90348ec to
dbea7ec
Compare
- Added handling for the two paths that need to be reported in the rename system call. - Introduced a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. - Collect more detailed information when an error occurs. - Replaced the `/proc/sys/fs/deepin-err-notify-enable` file with multiple netlink commands. - Reverted modifications to overlayfs; the `deepin_err_notify` mount option is no longer available. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
dbea7ec to
41996a9
Compare
- Added handling for the two paths that need to be reported in the rename system call. - Introduced a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. - Collect more detailed information when an error occurs. - Replaced the `/proc/sys/fs/deepin-err-notify-enable` file with multiple netlink commands. - Reverted modifications to overlayfs; the `deepin_err_notify` mount option is no longer available. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
41996a9 to
8ae4671
Compare
|
Your PR contains a single commit, but you have force-pushed that commit at least 25 times over a short period. GitHub notifies every watcher of repository activity, including each force-push and any automated responses from deepin-ci-bot. Stop this behavior immediately, redo your work locally until you have a final version, and then submit it. Thank you. Cc: @opsiff @Cryolitia |
- Added handling for the two paths that need to be reported in the rename system call. - Introduced a new macro `deepin_should_notify_ro_fs_err` to simplify error-checking code. - Collect more detailed information when an error occurs. - Replaced the `/proc/sys/fs/deepin-err-notify-enable` file with multiple netlink commands. - Reverted modifications to overlayfs; the `deepin_err_notify` mount option is no longer available. Link: deepin-community#1268 Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-opt-6.6.y-2
branch
from
8ae4671 to
cdfe7d0
Compare
deepin pr auto review我来对这个 git diff 进行代码审查：
具体修改建议：
[DEEPIN_ERR_NOTIFY_ATTR_FILENAME] = { .type = NLA_STRING, .len = PATH_MAX },
[DEEPIN_ERR_NOTIFY_ATTR_COMM] = { .type = NLA_STRING, .len = TASK_COMM_LEN },
if (!path_last || !path_last->path.dentry)
return -EINVAL;
#define MAX_PATH_COUNT 2
pr_debug("deepin_err_notify: Processing %d paths\n", path_last_count);
if (!capable(CAP_SYS_ADMIN))
return -EPERM;这些修改可以提高代码的健壮性、安全性和可维护性。 |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The netlink payload sends multiple filenames, inodes, and inode_parent flags by repeating the same attribute IDs without any explicit grouping or count field; consider using nested attributes or an explicit count attribute so userspace can reliably correlate and parse multi-path events.
- The func_name argument to deepin_check_and_notify_ro_fs_err() is no longer used in the notification path; consider either including it in the netlink event data or removing it from the API and all call sites to avoid carrying around a dead parameter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The netlink payload sends multiple filenames, inodes, and inode_parent flags by repeating the same attribute IDs without any explicit grouping or count field; consider using nested attributes or an explicit count attribute so userspace can reliably correlate and parse multi-path events.
- The func_name argument to deepin_check_and_notify_ro_fs_err() is no longer used in the notification path; consider either including it in the netlink event data or removing it from the API and all call sites to avoid carrying around a dead parameter.
## Individual Comments
### Comment 1
<location> `fs/namei.c:4079-4081` </location>
<code_context>
+ if (deepin_should_notify_ro_fs_err(error)) {
+ struct deepin_path_last path_last;
+
+ if (!deepin_lookup_path_or_parent(dfd, name, lookup_flags, &path_last)) {
+ deepin_check_and_notify_ro_fs_err(&path_last, "mknod");
+ deepin_put_path_last(&path_last);
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** `deepin_lookup_path_or_parent` treats `qstr.name` as a C-string, but `qstr` names are not guaranteed to be NUL-terminated.
In `deepin_lookup_path_or_parent()` you do:
```c
result_path_last->last = kstrdup((const char *)last.name, GFP_KERNEL);
```
and later `combine_path_and_last()` calls `strlen(last)` on that buffer. Since `struct qstr` only guarantees `name` + `len`, this can read past the allocated memory and lead to corruption.
Use:
- `kstrndup(last.name, last.len, GFP_KERNEL)` when duplicating, and
- propagate `last.len` (or an equivalent length) instead of calling `strlen(last)`.
Apply the same pattern anywhere `last.name` is handled as a C string.
</issue_to_address>
### Comment 2
<location> `fs/deepin_err_notify.c:289-298` </location>
<code_context>
- error = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_FILENAME, filename);
+ /* Add all filenames as separate attributes */
+ for (i = 0; i < event->count; i++) {
+ if (event->filenames[i]) {
+ error = nla_put_string(skb,
+ DEEPIN_ERR_NOTIFY_ATTR_FILENAME,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Sending multiple `DEEPIN_ERR_NOTIFY_ATTR_FILENAME` and inode attributes without nesting makes userspace parsing ambiguous.
The current encoding sends multiple `FILENAME` and inode-related attributes of the same type without any grouping. Netlink attributes are not positional arrays; generic parsers will usually keep only the last value for a given type, and even if userspace walks all attributes, there is no clear way to pair each filename with its inode/parent flag.
To avoid an ambiguous ABI, consider introducing a per-entry `NLA_NESTED` group (e.g. `DEEPIN_ERR_NOTIFY_ATTR_ENTRY`) that contains `FILENAME`, `INODE`, and `INODE_PARENT` for each item, so userspace can reliably correlate fields without relying on ordering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!deepin_lookup_path_or_parent(dfd, name, lookup_flags, &path_last)) { | ||
| deepin_check_and_notify_ro_fs_err(&path_last, "mknod"); | ||
| deepin_put_path_last(&path_last); |
There was a problem hiding this comment.
issue (bug_risk): deepin_lookup_path_or_parent treats qstr.name as a C-string, but qstr names are not guaranteed to be NUL-terminated.
In deepin_lookup_path_or_parent() you do:
result_path_last->last = kstrdup((const char *)last.name, GFP_KERNEL);and later combine_path_and_last() calls strlen(last) on that buffer. Since struct qstr only guarantees name + len, this can read past the allocated memory and lead to corruption.
Use:
kstrndup(last.name, last.len, GFP_KERNEL)when duplicating, and- propagate
last.len(or an equivalent length) instead of callingstrlen(last).
Apply the same pattern anywhere last.name is handled as a C string.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Initialize sysctl interface */ | ||
| deepin_err_notify_sysctl_init(); | ||
| pr_debug( |
There was a problem hiding this comment.
[nitpick] The log level was changed from pr_info to pr_debug for the successful initialization message at line 703-705. While this reduces log verbosity, it makes it harder to verify that the feature is properly initialized during system boot. Since this is a one-time initialization message, consider keeping it as pr_info for better observability, or at least document the rationale for downgrading to debug level.
| pr_debug( | |
| pr_info( |
| "rmdir"); | ||
| } | ||
| dput(dentry); | ||
| deepin_put_path_last(&path_last); |
There was a problem hiding this comment.
Memory leak when path lookup fails in the middle. When deepin_lookup_path_or_parent succeeds but the subsequent check at line 4296 (if (!path_last.last)) is false, the function returns without calling deepin_put_path_last(&path_last). This means the path reference acquired in deepin_lookup_path_or_parent is never released when path_last.last is not NULL. The cleanup call should be moved outside the conditional block or a call should be added to the else branch.
| deepin_put_path_last(&path_last); | |
| deepin_put_path_last(&path_last); | |
| } else { | |
| deepin_put_path_last(&path_last); |
| if (deepin_should_notify_ro_fs_err(error)) { | ||
| struct deepin_path_last path_last; | ||
|
|
||
| if (!deepin_lookup_path_or_parent(dfd, name, lookup_flags, &path_last)) { | ||
| if (!path_last.last) { | ||
| // File exists, notify error | ||
| deepin_check_and_notify_ro_fs_err(&path_last, | ||
| "unlink"); | ||
| } | ||
| dput(dentry); | ||
| deepin_put_path_last(&path_last); | ||
| } |
There was a problem hiding this comment.
Memory leak when path lookup fails in the middle. When deepin_lookup_path_or_parent succeeds but the subsequent check at line 4453 (if (!path_last.last)) is false, the function returns without calling deepin_put_path_last(&path_last). This means the path reference acquired in deepin_lookup_path_or_parent is never released when path_last.last is not NULL. The cleanup call should be moved outside the conditional block or a call should be added to the else branch.
| char *p = d_absolute_path( | ||
| &pl->path, path_bufs[i], PATH_MAX); |
There was a problem hiding this comment.
Using d_absolute_path() instead of d_path() changes the behavior when dealing with paths outside the root filesystem. d_absolute_path() returns an error for paths that escape the process's root, while the old code used d_path(). This could cause legitimate paths to fail conversion in containers or chroot environments. Consider whether this behavior change is intentional, and if so, handle the error case more gracefully by providing a fallback or logging.
|
|
||
| msg_head = genlmsg_put(skb, 0, 0, &deepin_err_notify_genl_family, 0, | ||
| DEEPIN_ERR_NOTIFY_CMD_NOTIFY); | ||
| DEEPIN_ERR_NOTIFY_CMD_ERR_ROFS); |
There was a problem hiding this comment.
The netlink command enum value changed from DEEPIN_ERR_NOTIFY_CMD_NOTIFY to DEEPIN_ERR_NOTIFY_CMD_ERR_ROFS (line 310), which is a breaking API change. Userspace applications expecting the old command will fail to parse messages. Additionally, the attributes were reordered and new ones added (lines 39-49). While the PR description mentions replacing the sysctl interface with netlink commands, it doesn't explicitly call out that this breaks the existing netlink message format. Document this breaking change clearly.
| /* Use GFP_ATOMIC because we might be called from atomic context */ | ||
| skb = genlmsg_new(msg_size, GFP_ATOMIC); |
There was a problem hiding this comment.
The GFP_ATOMIC flag is used here, but the function comment at line 301 states "Use GFP_ATOMIC because we might be called from atomic context". However, all callers appear to be in syscall error paths after operations like mnt_drop_write(), which are not atomic contexts. Using GFP_ATOMIC unnecessarily increases the risk of allocation failures. Consider using GFP_KERNEL for better reliability, or clearly document which atomic contexts this function is called from.
| /* Use GFP_ATOMIC because we might be called from atomic context */ | |
| skb = genlmsg_new(msg_size, GFP_ATOMIC); | |
| /* Use GFP_KERNEL since all callers are in process context (not atomic) */ | |
| skb = genlmsg_new(msg_size, GFP_KERNEL); |
| const struct deepin_path_last path_lasts[2] = { | ||
| { .path = *old_path, .last = old_last->name }, | ||
| { .path = *new_path, .last = new_last->name } |
There was a problem hiding this comment.
Inconsistent memory management semantics. The deepin_put_path_last function at line 468 frees path_last->last with kfree(), but deepin_notify_rename_ro_fs_err at lines 670-672 constructs a deepin_path_last with old_last->name and new_last->name which are not heap-allocated strings. After prepare_and_notify_fs_error processes these, if someone calls deepin_put_path_last, it will attempt to kfree() stack or dentry memory. The function should document that callers must not use deepin_put_path_last on stack-constructed deepin_path_last structures, or the structure should be redesigned to indicate ownership.
| if (event->filenames[i]) | ||
| msg_size += | ||
| nla_total_size(strlen(event->filenames[i]) + 1); |
There was a problem hiding this comment.
The loop at lines 289-293 calls strlen() on each filename, but line 290 checks if (event->filenames[i]) only inside the loop after starting to calculate message size. If any filename is NULL, the strlen() at line 292 will dereference NULL. The check should be moved before the msg_size += operation, or the loop structure should be restructured to skip NULL entries entirely when calculating size.
| if (event->filenames[i]) | |
| msg_size += | |
| nla_total_size(strlen(event->filenames[i]) + 1); | |
| if (!event->filenames[i]) | |
| continue; | |
| msg_size += nla_total_size(strlen(event->filenames[i]) + 1); |
| error = filename_lookup(dfd, name, flags, &result_path, NULL); | ||
| if (error == -ENOENT) { | ||
| error = filename_parentat(dfd, name, flags, &parent, &last, &type); | ||
| if (error) |
There was a problem hiding this comment.
[nitpick] When handling the -ENOENT case, if filename_parentat() returns an error at line 5374, the function returns early at line 5375 without calling path_put(&result_path). However, since filename_lookup() returned -ENOENT at line 5372, the result_path was never successfully initialized and doesn't hold a reference. This is correct behavior, but the code flow could be clearer. Consider adding a comment to clarify that result_path is not initialized when filename_lookup returns -ENOENT.
| if (error) | |
| if (error) | |
| /* No need to call path_put(&result_path) here: | |
| * result_path was never initialized when filename_lookup() | |
| * returns -ENOENT. | |
| */ |
| static int deepin_err_notify_enable __read_mostly; | ||
|
|
||
| int deepin_err_notify_enabled(void) | ||
| inline int deepin_err_notify_enabled(void) |
There was a problem hiding this comment.
[nitpick] The inline keyword for deepin_err_notify_enabled() at line 73 may not provide meaningful performance benefit since this function is not in a hot path (it's only called during error handling), and modern compilers make their own inlining decisions. The inline keyword without static in a .c file can also lead to multiple definitions if the function is used across compilation units. Consider removing the inline keyword or making the function static inline and moving it to a header if it needs to be inlined.
| inline int deepin_err_notify_enabled(void) | |
| int deepin_err_notify_enabled(void) |
3 participants
fs: Enhanced error notification mechanism for read-only filesystems
deepin_should_notify_ro_fs_errto simplify error-checking code./proc/sys/fs/deepin-err-notify-enablefile with multiple netlink commands.deepin_err_notifymount option is no longer available.Summary by Sourcery
Refine Deepin filesystem error notifications for read-only failures and migrate configuration and reporting to a richer Generic Netlink-based mechanism.
New Features:
Enhancements:
Build: