fs/deepin_err_notify: Refactor the message construction and sending logic for file system error notifications #1377
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideRefactors Deepin filesystem error notification to use a unified event data struct, a generic notify_fs_error helper supporting multiple filenames and inode metadata, and a new prepare_and_notify_fs_error that gathers path/credential info before sending netlink multicast messages, while removing the old single-filename notification API and placeholder declarations. Sequence diagram for prepare_and_notify_fs_error and notify_fs_error interactionsequenceDiagram
participant Caller as caller_module
participant DEN as deepin_err_notify_module
participant Netlink as netlink_subsystem
Caller->>DEN: prepare_and_notify_fs_error(path_lasts, path_last_count)
activate DEN
DEN->>DEN: validate_parameters(path_lasts, path_last_count)
DEN->>DEN: collect_process_info()
DEN->>DEN: collect_credentials()
loop for_each_path_last
DEN->>DEN: allocate_path_buffer()
alt last_is_present
DEN->>DEN: combine_path_and_last(buffer, path, last)
DEN->>DEN: record_parent_inode()
else complete_path
DEN->>DEN: d_absolute_path(path, buffer, PATH_MAX)
DEN->>DEN: record_file_inode()
end
end
alt at_least_one_valid_filename
DEN->>DEN: notify_fs_error(event)
DEN->>DEN: build_netlink_message(event)
DEN->>Netlink: genlmsg_multicast(skb)
Netlink-->>DEN: status_or_ESRCH
else no_valid_filename
DEN->>DEN: log_no_valid_filenames()
end
DEN->>DEN: free_path_buffers()
deactivate DEN
DEN-->>Caller: return error_code
Class diagram for deepin filesystem error notification refactorclassDiagram
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_path_last {
struct path path
const char *last
}
class deepin_err_notify_module {
-bool deepin_err_notify_initialized
-int deepin_err_notify_enable
-struct genl_family deepin_err_notify_genl_family
+int deepin_err_notify_should_send()
+int prepare_and_notify_fs_error(const deepin_path_last *path_lasts, int path_last_count)
+int notify_fs_error(const deepin_fs_err_event_data *event)
+char *combine_path_and_last(char *buffer, const struct path *path, const char *last)
}
deepin_err_notify_module "1" o-- "1" deepin_fs_err_event_data : builds_and_uses
deepin_err_notify_module "1" --> "*" deepin_path_last : consumes
deepin_err_notify_module : uses_genlmsg_multicast()
deepin_err_notify_module : uses_nla_put_*_helpers()
deepin_err_notify_module : uses_d_absolute_path()
deepin_err_notify_module : uses_current_task_context()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. DetailsInstructions 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. |
![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 found some issues that need to be addressed.
- The
deepin_fs_err_event_datastruct comment refers to@filename_countwhile the actual field is namedcount; update the documentation to match the code to avoid confusion for future maintainers. - The new netlink paths use
GFP_KERNELforgenlmsg_new/genlmsg_multicast, whereas the old implementation explicitly usedGFP_NOFSto avoid filesystem recursion; consider whetherGFP_NOFSis still required in these call sites. - In
notify_fs_error,event->countdrives both filename and inode/parent-flag emission, butfilenames[i]may be NULL whileinodes[i]/inode_is_parent[i]are still sent; if the arrays can be partially populated, consider makingcountreflect the number of fully valid entries or guard inode attributes on the corresponding filename being present.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `deepin_fs_err_event_data` struct comment refers to `@filename_count` while the actual field is named `count`; update the documentation to match the code to avoid confusion for future maintainers.
- The new netlink paths use `GFP_KERNEL` for `genlmsg_new`/`genlmsg_multicast`, whereas the old implementation explicitly used `GFP_NOFS` to avoid filesystem recursion; consider whether `GFP_NOFS` is still required in these call sites.
- In `notify_fs_error`, `event->count` drives both filename and inode/parent-flag emission, but `filenames[i]` may be NULL while `inodes[i]`/`inode_is_parent[i]` are still sent; if the arrays can be partially populated, consider making `count` reflect the number of fully valid entries or guard inode attributes on the corresponding filename being present.
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:338` </location>
<code_context>
- /* Use GFP_NOFS to avoid recursion in file system operations. */
- skb = genlmsg_new(msg_size, GFP_NOFS);
+ skb = genlmsg_new(msg_size, GFP_KERNEL);
if (!skb) {
- pr_err("deepin_err_notify: Failed to allocate netlink message\n");
</code_context>
<issue_to_address>
**issue (bug_risk):** Using GFP_KERNEL in filesystem error path can deadlock under memory pressure.
Since this runs in filesystem paths where reclaim may already hold FS locks, allocating with GFP_KERNEL (both for the skb and the kmalloc(PATH_MAX) in prepare_and_notify_fs_error()) risks re-entering the filesystem and deadlocking under memory pressure. Please keep these allocations as GFP_NOFS (as before), unless you can guarantee this is never called under filesystem locks.
</issue_to_address>
### Comment 2
<location> `fs/deepin_err_notify.c:410` </location>
<code_context>
+ * For other error codes, the skb is NOT consumed and must be freed
+ * by the caller.
+ */
+ error = genlmsg_multicast(&deepin_err_notify_genl_family, skb, 0, 0,
+ GFP_KERNEL);
+ if (error) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The skb ownership assumptions around genlmsg_multicast() may cause double frees.
Relying on genlmsg_multicast() not consuming the skb for all non-ESRCH errors is brittle: kernel helpers may free the skb on additional error paths, or this behavior may change across versions. If that happens, the kfree_skb(skb) in err_out becomes a double free. To make this robust, treat the skb as potentially consumed after genlmsg_multicast() (e.g., set skb = NULL after the call and guard kfree_skb() with a NULL check, or only free it when you are certain it was not consumed for your target kernel versions).
</issue_to_address>
### Comment 3
<location> `fs/deepin_err_notify.c:83` </location>
<code_context>
+ * This structure consolidates all the parameters needed for sending
+ * filesystem error notifications via netlink.
+ */
+struct deepin_fs_err_event_data {
+ const char **filenames;
+ int count;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the filesystem error reporting code to use a small per-entry struct and simpler iteration/error paths instead of parallel arrays and multiple gotos to make the logic easier to follow and maintain.
You can simplify this without losing any functionality by tightening the data model and control flow around “entries” rather than parallel arrays, and by collapsing some of the error‑handling paths.
### 1. Replace parallel arrays with a per‑entry struct
Right now `deepin_fs_err_event_data` and `prepare_and_notify_fs_error()` maintain three parallel arrays (`filenames`, `inodes`, `inode_parent_flags`) plus `count`. This makes the logic in both functions harder to follow.
You can keep all behavior but define a small per‑entry struct and use an array of that (size 2) in the stack code:
```c
struct deepin_fs_err_entry {
const char *filename;
ino_t ino;
bool inode_is_parent;
};
struct deepin_fs_err_event_data {
const struct deepin_fs_err_entry *entries;
int count;
pid_t pid;
pid_t ppid;
const char *comm;
uid_t uid;
gid_t gid;
};
```
Then, in `prepare_and_notify_fs_error()`:
```c
static int prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts,
int path_last_count)
{
struct deepin_fs_err_entry entries[2];
char *path_bufs[2] = { NULL, NULL };
int count = 0;
int i;
/* ... validate path_lasts, setup pid/uid/gid, etc ... */
for (i = 0; i < path_last_count; i++) {
const struct deepin_path_last *pl = &path_lasts[i];
struct deepin_fs_err_entry *e = &entries[count];
char *buf;
if (!pl->path.dentry)
continue;
buf = kmalloc(PATH_MAX, GFP_KERNEL);
if (!buf)
continue;
path_bufs[count] = buf;
if (pl->last) {
char *combined = combine_path_and_last(buf, &pl->path, pl->last);
if (!combined)
continue;
e->filename = combined;
if (pl->path.dentry->d_inode) {
e->ino = pl->path.dentry->d_inode->i_ino;
e->inode_is_parent = true;
}
} else {
char *p = d_absolute_path(&pl->path, buf, PATH_MAX);
if (IS_ERR(p)) {
pr_debug("deepin_err_notify: d_absolute_path failed: %ld\n",
PTR_ERR(p));
continue;
}
e->filename = p;
if (pl->path.dentry->d_inode)
e->ino = pl->path.dentry->d_inode->i_ino;
e->inode_is_parent = false;
}
count++;
}
if (count > 0) {
struct deepin_fs_err_event_data event = {
.entries = entries,
.count = count,
.pid = pid,
.ppid = ppid,
.comm = comm,
.uid = uid,
.gid = gid,
};
error = notify_fs_error(&event);
}
for (i = 0; i < 2; i++)
kfree(path_bufs[i]);
return error;
}
```
This keeps the same semantics but removes the need to keep three arrays and `count` in sync manually, and removes the type drift (`unsigned long` vs `ino_t`).
If you prefer, you can go one step further and hide the conversion logic into a helper:
```c
static bool deepin_fill_fs_err_entry(const struct deepin_path_last *pl,
struct deepin_fs_err_entry *entry,
char *buf)
{
/* path from pl + set entry->filename/ino/inode_is_parent */
}
```
Then `prepare_and_notify_fs_error()` just loops and calls this helper.
### 2. Simplify `notify_fs_error()` iteration and error flow
Once you have `entries[]`, `notify_fs_error()` can be simpler to reason about:
```c
static int notify_fs_error(const struct deepin_fs_err_event_data *event)
{
int i, msg_size;
struct sk_buff *skb;
void *msg_head;
int error;
if (!event || !event->entries || event->count <= 0 || !event->comm)
return -EINVAL;
msg_size = nla_total_size(sizeof(u32)) /* PID */
+ nla_total_size(sizeof(u32)) /* PPID */
+ nla_total_size(strlen(event->comm) + 1)
+ nla_total_size(sizeof(u32)) /* UID */
+ nla_total_size(sizeof(u32)); /* GID */
for (i = 0; i < event->count; i++) {
const struct deepin_fs_err_entry *e = &event->entries[i];
if (e->filename)
msg_size += nla_total_size(strlen(e->filename) + 1);
msg_size += nla_total_size(sizeof(u64)); /* INODE */
msg_size += nla_total_size(sizeof(u8)); /* INODE_PARENT */
}
skb = genlmsg_new(msg_size, GFP_KERNEL);
if (!skb)
return -ENOMEM;
msg_head = genlmsg_put(skb, 0, 0, &deepin_err_notify_genl_family, 0,
DEEPIN_ERR_NOTIFY_CMD_ERR_ROFS);
if (!msg_head) {
error = -EMSGSIZE;
goto out_free_skb;
}
for (i = 0; i < event->count; i++) {
const struct deepin_fs_err_entry *e = &event->entries[i];
if (e->filename) {
error = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_FILENAME,
e->filename);
if (error)
goto out_free_skb;
}
error = nla_put_u64_64bit(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE,
e->ino, 0);
if (error)
goto out_free_skb;
error = nla_put_u8(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT,
e->inode_is_parent);
if (error)
goto out_free_skb;
}
/* PID/PPID/COMM/UID/GID as you already do */
genlmsg_end(skb, msg_head);
error = genlmsg_multicast(&deepin_err_notify_genl_family, skb, 0, 0,
GFP_KERNEL);
if (error && error != -ESRCH) {
pr_err("deepin_err_notify: Multicast failed: %d\n", error);
goto out_free_skb;
}
return 0;
out_free_skb:
kfree_skb(skb);
return error;
}
```
This:
- Uses a single conceptual iteration over `entries` for sizing and another for emission (no parallel arrays).
- Collapses the attribute and header error paths into a single `out_free_skb` label.
- Keeps the multicast semantics intact while reducing the multi‑label control flow.
These two changes (per‑entry struct + simplified notify) should address most of the complexity and bookkeeping concerns while preserving the feature set and behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
electricface
force-pushed
the
swt/err-notify-p4
branch
3 times, most recently
from
6ba9996 to
1def06a
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- prepare_and_notify_fs_error() relies on combine_path_and_last(), but combine_path_and_last() is still a stub that always returns NULL, which means the new multi-path flow will never produce combined filenames and may silently skip notifications; either implement it in this change or guard its use to avoid behavior regressions.
- The new notify_fs_error() path allocates and sends the netlink skb with GFP_KERNEL instead of GFP_NOFS as used previously, which can reintroduce filesystem recursion and allocation deadlock risks in error paths; consider keeping GFP_NOFS for these allocations.
- prepare_and_notify_fs_error() stores inode values in an unsigned long[2] but passes them as ino_t* to notify_fs_error(); using ino_t[2] instead would avoid potential size/representation mismatches on 32-bit or non-standard architectures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- prepare_and_notify_fs_error() relies on combine_path_and_last(), but combine_path_and_last() is still a stub that always returns NULL, which means the new multi-path flow will never produce combined filenames and may silently skip notifications; either implement it in this change or guard its use to avoid behavior regressions.
- The new notify_fs_error() path allocates and sends the netlink skb with GFP_KERNEL instead of GFP_NOFS as used previously, which can reintroduce filesystem recursion and allocation deadlock risks in error paths; consider keeping GFP_NOFS for these allocations.
- prepare_and_notify_fs_error() stores inode values in an unsigned long[2] but passes them as ino_t* to notify_fs_error(); using ino_t[2] instead would avoid potential size/representation mismatches on 32-bit or non-standard architectures.
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:411` </location>
<code_context>
+ * For other error codes, the skb is NOT consumed and must be freed
+ * by the caller.
+ */
+ error = genlmsg_multicast(&deepin_err_notify_genl_family, skb, 0, 0,
+ GFP_KERNEL);
+ if (error) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential double-free: genlmsg_multicast() may already consume skb on error paths.
This relies on genlmsg_multicast() only consuming the skb on success or -ESRCH, but netlink multicast helpers usually consume the skb on all paths, including most errors. In the non-ESRCH error branch you jump to `err_out:` and call `kfree_skb(skb)`, which risks a double free if genlmsg_multicast() already freed it. Please align with the documented ownership rules: either let genlmsg_multicast() always own and free the skb, or only free it in the explicitly documented non-consumed cases.
</issue_to_address>
### Comment 2
<location> `fs/deepin_err_notify.c:339-348` </location>
<code_context>
+ skb = genlmsg_new(msg_size, GFP_KERNEL);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using GFP_KERNEL instead of GFP_NOFS for filesystem error notifications may reintroduce recursion risks.
The earlier code used GFP_NOFS for both skb allocation and multicast send specifically to avoid reclaim calling back into filesystem code while already in a filesystem path. Using GFP_KERNEL here can reintroduce reclaim-induced recursion or deadlock when errors are reported from deep in the VFS. Unless there’s a clear justification for changing this, please keep GFP_NOFS or carefully reassess the required gfp flags for all call sites of prepare_and_notify_fs_error().
Suggested implementation:
```c
skb = genlmsg_new(msg_size, GFP_NOFS);
```
Review the rest of `fs/deepin_err_notify.c` to ensure:
1. All skb allocations related to filesystem error notifications use `GFP_NOFS` (or an explicitly justified alternative).
2. Any `genlmsg_multicast*()` or similar send paths that previously used `GFP_NOFS` are not changed to `GFP_KERNEL` without a clear deadlock/recursion analysis.
If other call sites of `prepare_and_notify_fs_error()` were modified to use `GFP_KERNEL` (directly or indirectly), they should be reverted to `GFP_NOFS` or analyzed and documented accordingly.
</issue_to_address>
### Comment 3
<location> `fs/deepin_err_notify.c:460` </location>
<code_context>
+ char *path_bufs[2] = { NULL, NULL };
+ const char *filenames[2] = { "", "" };
+ int count = 0;
+ unsigned long inodes[2] = { 0, 0 };
+ bool inode_parent_flags[2] = { false, false };
+ int i = 0;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Possible inode truncation on 32-bit architectures when sending as u64.
`inodes` is `unsigned long[2]` but is passed as `const ino_t *` and serialized with `nla_put_u64_64bit()`. On 32-bit builds, `unsigned long`/`ino_t` may be 32-bit, so values will be zero-extended into the u64 attribute and 64-bit inode numbers can be truncated. If you need full 64-bit inodes, use `u64 inodes[2]` and adjust the struct field type accordingly to avoid architecture-dependent loss of data.
Suggested implementation:
```c
/* Path conversion variables */
char *path_bufs[2] = { NULL, NULL };
const char *filenames[2] = { "", "" };
int count = 0;
u64 inodes[2] = { 0, 0 };
bool inode_parent_flags[2] = { false, false };
int i = 0;
```
To fully implement your comment and avoid architecture-dependent truncation:
1. Update the event struct (likely something like `struct deepin_err_event` in `fs/deepin_err_notify.c` or a nearby header) so that:
- `inodes` is declared as `u64 inodes[2];` (or `u64 *inodes;` if dynamically sized) instead of `unsigned long`/`ino_t`.
- `inode_is_parent` remains `bool inode_is_parent[2];` (or matching the local `inode_parent_flags` type).
2. Ensure that wherever `event->inodes` is assigned (where the event is produced), you use 64-bit inode values:
- For example, if you currently assign from `struct inode *inode`, use `event->inodes[i] = inode->i_ino;` and make sure `i_ino` is cast or stored in `u64` if necessary.
3. In this function, when copying from the event to the local arrays (the code is not shown in the snippet), ensure assignments are type-consistent:
- Replace usages like `inodes[i] = event->inodes[i];` where the local `inodes` is `u64[2]` and the struct field has been updated to `u64[2]` as well.
4. The `nla_put_u64_64bit()` calls that serialize these values over netlink should now pass the `u64` `inodes` array (or `event->inodes`) directly, without casting from `unsigned long` or `ino_t`.
</issue_to_address>
### Comment 4
<location> `fs/deepin_err_notify.c:68-77` </location>
<code_context>
#define DEEPIN_ERR_NOTIFY_CMD_MAX (__DEEPIN_ERR_NOTIFY_CMD_MAX - 1)
+/**
+ * struct deepin_fs_err_event_data - Filesystem error event data for netlink
+ * @filenames: Array of filenames involved in the error
+ * @filename_count: Number of filenames in the array (also used for inode count)
+ * @inodes: Array of inode numbers corresponding to the filenames
+ * @inode_is_parent: Array of bool flags indicating if inode is from parent dentry
+ * @pid: Process ID that triggered the error
+ * @ppid: Parent Process ID
+ * @comm: Process short name (15 chars max)
+ * @uid: User ID
+ * @gid: Group ID
+ *
+ * This structure consolidates all the parameters needed for sending
+ * filesystem error notifications via netlink.
+ */
+struct deepin_fs_err_event_data {
+ const char **filenames;
+ int count;
+ const ino_t *inodes;
+ const bool *inode_is_parent;
</code_context>
<issue_to_address>
**nitpick:** Struct documentation is out of sync with the field name.
The doc string describes `@filename_count`, but the field is defined as `int count;`. Please align the documented parameter name with the actual field name to avoid confusion and to comply with kernel-doc conventions.
</issue_to_address>
### Comment 5
<location> `fs/deepin_err_notify.c:83` </location>
<code_context>
+ * This structure consolidates all the parameters needed for sending
+ * filesystem error notifications via netlink.
+ */
+struct deepin_fs_err_event_data {
+ const char **filenames;
+ int count;
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the parallel filename/inode/flag arrays with a small per-file struct and iterating over that once in both preparation and netlink send to simplify the data flow.
You can drop a layer of indirection and the parallel arrays without changing behavior by moving to a small “file info” struct and iterating over that in both preparation and netlink send.
For example, instead of:
```c
struct 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;
};
```
with:
```c
const char *filenames[2];
unsigned long inodes[2];
bool inode_parent_flags[2];
...
filenames[count] = ...;
inodes[count] = ...;
inode_parent_flags[count] = ...;
count++;
```
and then separate loops in `notify_fs_error()`, you can keep the same feature set but simplify to:
```c
struct deepin_fs_err_file {
const char *name;
u64 ino;
bool is_parent;
};
struct deepin_fs_err_event_data {
struct deepin_fs_err_file files[2];
int count;
pid_t pid;
pid_t ppid;
const char *comm;
uid_t uid;
gid_t gid;
};
```
`prepare_and_notify_fs_error()` then becomes a single-index fill, avoiding parallel arrays and “i vs count” confusion:
```c
for (i = 0; i < path_last_count; i++) {
const struct deepin_path_last *pl = &path_lasts[i];
struct deepin_fs_err_file *f;
if (!pl->path.dentry)
continue;
path_bufs[i] = __getname();
if (!path_bufs[i])
continue;
f = &event.files[event.count];
if (pl->last) {
char *combined = combine_path_and_last(path_bufs[i], &pl->path, pl->last);
if (!combined)
continue;
f->name = combined;
if (pl->path.dentry->d_inode)
f->ino = pl->path.dentry->d_inode->i_ino;
f->is_parent = true;
} else {
char *p = d_absolute_path(&pl->path, path_bufs[i], PATH_MAX);
if (IS_ERR(p))
continue;
f->name = p;
if (pl->path.dentry->d_inode)
f->ino = pl->path.dentry->d_inode->i_ino;
f->is_parent = false;
}
event.count++;
}
```
`notify_fs_error()` can then be linearized into a single loop over `files[]`, avoiding multiple passes and parallel arrays:
```c
static int notify_fs_error(const struct deepin_fs_err_event_data *event)
{
int i, msg_size;
struct sk_buff *skb;
void *msg_head;
int error;
if (!event || event->count <= 0 || !event->comm)
return -EINVAL;
msg_size = nla_total_size(sizeof(u32)) /* PID */
+ nla_total_size(sizeof(u32)) /* PPID */
+ nla_total_size(strlen(event->comm) + 1)
+ nla_total_size(sizeof(u32)) /* UID */
+ nla_total_size(sizeof(u32)); /* GID */
for (i = 0; i < event->count; i++) {
if (event->files[i].name)
msg_size += nla_total_size(strlen(event->files[i].name) + 1);
msg_size += nla_total_size(sizeof(u64)); /* INODE */
msg_size += nla_total_size(sizeof(u8)); /* INODE_PARENT */
}
skb = genlmsg_new(msg_size, GFP_KERNEL);
...
for (i = 0; i < event->count; i++) {
const struct deepin_fs_err_file *f = &event->files[i];
if (f->name) {
error = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_FILENAME,
f->name);
if (error)
goto attr_err_out;
}
error = nla_put_u64_64bit(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE,
f->ino, 0);
if (error)
goto attr_err_out;
error = nla_put_u8(skb, DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT,
f->is_parent ? 1 : 0);
if (error)
goto attr_err_out;
}
...
}
```
This keeps all current functionality (multiple filenames, inode + parent flag, process/cred info, netlink semantics) but:
- removes the parallel `filenames[]` / `inodes[]` / `inode_parent_flags[]` arrays;
- removes the separate `i` vs `count` indexing, using only `event.count` as the size;
- collapses the multiple netlink sizing/sending loops into a single loop over a compact per-file struct.
If you want to further trim branching noise, you can also tighten validation now that `event` is constructed locally:
```c
/* prepare_and_notify_fs_error */
struct deepin_fs_err_event_data event = {
.count = 0,
.pid = pid,
.ppid = ppid,
.comm = comm,
.uid = uid,
.gid = gid,
};
/* notify_fs_error */
if (WARN_ON_ONCE(event->count > ARRAY_SIZE(event->files)))
return -EINVAL;
if (event->count == 0)
return 0;
```
This keeps the defensive behavior while avoiding repeated null/array checks that are impossible in the current construction pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
electricface
force-pushed
the
swt/err-notify-p4
branch
from
1def06a to
d4844ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Deepin filesystem error notification system to support richer error event data. The changes introduce a structured approach to capturing and sending filesystem errors via netlink, including support for multiple filenames, inode information, and process credentials. However, the refactoring is incomplete, with a critical stub function that will cause the feature to fail in certain code paths.
Key Changes:
- Introduced
deepin_fs_err_event_datastructure to consolidate error event parameters (filenames, inodes, process info) - Replaced
deepin_send_ro_fs_err_notificationwith newnotify_fs_errorfunction that handles multiple filenames and extended metadata - Enhanced
prepare_and_notify_fs_errorto extract full paths and inode information fromdeepin_path_laststructures
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| fs/internal.h | Removed three function declarations: deepin_get_path_for_err_notify, deepin_err_notify_enabled, and deepin_send_ro_fs_err_notification to internalize implementation details |
| fs/deepin_err_notify.c | Major refactoring: added event data structure, implemented new notification function with extended attributes (inode, ppid, uid, gid), and rewrote path/filename handling logic with inode extraction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Use GFP_NOFS to avoid recursion in file system operations. */ | ||
| skb = genlmsg_new(msg_size, GFP_NOFS); | ||
| skb = genlmsg_new(msg_size, GFP_KERNEL); |
There was a problem hiding this comment.
The GFP flag was changed from GFP_NOFS to GFP_KERNEL. The original comment in the removed code stated "Use GFP_NOFS to avoid recursion in file system operations." This change could introduce filesystem recursion issues if this notification is triggered from within filesystem code paths that hold locks or are in contexts where filesystem operations cannot recurse. Since this is called from deepin_check_and_notify_ro_fs_err which appears to be invoked during filesystem error handling, GFP_NOFS is likely more appropriate to prevent potential deadlocks.
Suggested change
| skb = genlmsg_new(msg_size, GFP_KERNEL); | |
| skb = genlmsg_new(msg_size, GFP_NOFS); |
Contributor
Author
There was a problem hiding this comment.
继续使用GFP_KERNEL
核心问题：什么时候需要 GFP_NOFS？
使用 GFP_NOFS 的唯一原因是防止死锁。当内存紧张时，分配器可能会：
尝试回收内存 → 回写脏页 → 调用文件系统的 write 路径
如果此时持有文件系统锁，就会死锁
调用文件系统的 write 一般不会遇到 EROFS 吧，我的代码也没有持有文件系统锁。
我的函数都是处理出错 EROFS 的，是锁都释放的情况。
| * by the caller. | ||
| */ | ||
| error = genlmsg_multicast(&deepin_err_notify_genl_family, skb, 0, 0, | ||
| GFP_KERNEL); |
There was a problem hiding this comment.
The GFP flag was changed from GFP_NOFS to GFP_KERNEL. Similar to the allocation above, this multicast operation occurs in a filesystem error handling context where GFP_NOFS was intentionally used to prevent recursion. The original implementation used GFP_NOFS here to match the allocation context and avoid potential deadlocks when called from filesystem operations.
Suggested change
| GFP_KERNEL); | |
| GFP_NOFS); |
Contributor
Author
There was a problem hiding this comment.
继续使用GFP_KERNEL
Comment on lines
+496
to
+550
| path_bufs[i] = __getname(); | ||
| if (!path_bufs[i]) | ||
| continue; | ||
|
|
||
| /* Build the filename */ | ||
| if (pl->last) { | ||
| /* Complete path not obtained, need to combine path + last */ | ||
| char *combined_path = combine_path_and_last( | ||
| path_bufs[i], &pl->path, pl->last); | ||
| if (!combined_path) { | ||
| pr_warn( | ||
| "deepin_err_notify: combine_path_and_last failed for path_last[%d]\n", | ||
| i); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When __getname() succeeds at line 496 but subsequent operations fail (either combine_path_and_last returns NULL or d_absolute_path returns an error), the allocated buffer is still correctly freed in the cleanup section at lines 576-579 because cleanup iterates through all possible buffer positions (0 and 1) and frees any non-NULL buffer. However, the early continue after allocation failure bypasses incrementing count, which means a successfully allocated buffer that encounters an error won't be included in the notification but will still be properly freed. This logic is correct but not immediately obvious.
Contributor
Author
There was a problem hiding this comment.
不需要修改。理由如下：
内存安全：清理逻辑是正确的，不会造成内存泄漏
逻辑清晰：count 表示有效的通知数量，i 表示迭代索引，两者职责明确
fs/deepin_err_notify.c
Outdated
| /* File exists, use the file's own inode */ | ||
| if (target_dentry->d_inode) | ||
| inodes[count] = | ||
| target_dentry->d_inode->i_ino; |
There was a problem hiding this comment.
The inode_parent_flags array is not being properly set in the else branch (when pl->last is NULL). When a complete path is obtained and d_absolute_path succeeds, the code increments count at line 537 but never sets inode_parent_flags[count] before incrementing. This means the flag retains its initial value of false from line 461, which is correct, but the logic is implicit and fragile. For clarity and correctness, explicitly set inode_parent_flags[count] = false; before line 537 to make the intent clear.
Suggested change
| target_dentry->d_inode->i_ino; | |
| target_dentry->d_inode->i_ino; | |
| inode_parent_flags[count] = false; |
| int deepin_lookup_path_or_parent(int dfd, struct filename *name, | ||
| unsigned int flags, | ||
| struct deepin_path_last *result_path_last); | ||
|
|
There was a problem hiding this comment.
The declaration of deepin_err_notify_enabled() was removed from the header, but the function is still defined and used within the same file. This makes the function internal to deepin_err_notify.c. If this was intentionally changed from an exported function to a static internal function, the function definition should also be marked as static at line 108. If it needs to remain accessible from other files, the declaration should be retained in the header.
Comment on lines
+313
to
+353
| if (!event || !event->filenames || event->count <= 0 || !event->comm || | ||
| !event->inodes || !event->inode_is_parent) { |
There was a problem hiding this comment.
The validation requires !event->inodes and !event->inode_is_parent to be considered invalid, but these are never expected to be NULL in valid usage since they should always point to stack-allocated arrays. A more appropriate check would be to validate that for a valid event, these arrays contain meaningful data, or simply rely on the other checks (event, event->filenames, event->count > 0, event->comm) which are more critical. The current checks for inodes and inode_is_parent being NULL may never actually catch real errors since they're typically stack arrays, not pointers that could be NULL.
Suggested change
| if (!event || !event->filenames || event->count <= 0 || !event->comm || | |
| !event->inodes || !event->inode_is_parent) { | |
| if (!event || !event->filenames || event->count <= 0 || !event->comm) { |
fs/deepin_err_notify.c
Outdated
Comment on lines
457
to
491
| char *path_bufs[2] = { NULL, NULL }; | ||
| const char *filenames[2] = { "", "" }; | ||
| int count = 0; | ||
| unsigned long inodes[2] = { 0, 0 }; | ||
| bool inode_parent_flags[2] = { false, false }; | ||
| int i = 0; | ||
| int error = 0; | ||
|
|
||
| /* Validate input parameters */ | ||
| if (!path_lasts || path_last_count <= 0) { | ||
| pr_warn( | ||
| "deepin_err_notify: Invalid parameters: path_lasts=%p, path_last_count=%d\n", | ||
| path_lasts, path_last_count); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| /* Only handle 1 or 2 path_lasts */ | ||
| if (path_last_count > 2) { | ||
| pr_warn( | ||
| "deepin_err_notify: Invalid path_last_count=%d (must be 1 or 2)\n", | ||
| path_last_count); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| /* Get basic process information from current context */ | ||
| pid = current->pid; | ||
| ppid = current->real_parent ? current->real_parent->pid : 0; | ||
| comm = current->comm; /* Short name, limited to 15 characters */ | ||
|
|
||
| /* Get credentials */ | ||
| uid = from_kuid(&init_user_ns, current_uid()); | ||
| gid = from_kgid(&init_user_ns, current_gid()); | ||
|
|
||
| /* Convert each path_last to a filename string and extract inode */ | ||
| for (i = 0; i < path_last_count; i++) { |
There was a problem hiding this comment.
The arrays path_bufs, filenames, inodes, and inode_parent_flags are hardcoded to size 2, and there's a validation check at lines 474-479 limiting path_last_count to 2. However, the loop at lines 491-540 uses path_last_count directly without bounds checking against the array size. If the validation logic is bypassed or modified in the future, this could lead to buffer overflows. Consider using a named constant for the maximum array size (e.g., MAX_PATH_LASTS) and using it consistently in the array declarations, validation check, and loop bounds.
fs/deepin_err_notify.c
Outdated
| * filesystem error information to userspace listeners. | ||
| * Multiple filenames are sent as separate attributes. | ||
| * | ||
| * Returns: 0 on success, negative error code on failure |
There was a problem hiding this comment.
The function documentation states "Returns: 0 on success, negative error code on failure" but the implementation returns 0 even when -ESRCH (no listeners) occurs at line 419. While this may be intentional (treating "no listeners" as success), the documentation should clarify that -ESRCH is treated as success, or the return value description should note that 0 is returned even when there are no listeners to receive the notification.
Suggested change
| * Returns: 0 on success, negative error code on failure | |
| * Returns: 0 on success (including when there are no listeners to receive | |
| * the notification, i.e., when -ESRCH occurs), negative error code on other | |
| * failures. |
fs/deepin_err_notify.c
Outdated
|
|
||
| filenames[count] = combined_path; | ||
|
|
||
| /* File doesn't exist, use parent dentry's inode if available */ |
There was a problem hiding this comment.
The comment states "File doesn't exist, use parent dentry's inode if available" but this is misleading. When pl->last is non-NULL, it means the lookup didn't complete successfully (the target may not exist), but the code is using target_dentry which is set to pl->path.dentry. This represents the parent directory, not the non-existent file. The comment should be clarified to say "Use parent directory's inode since target doesn't exist" to be more accurate.
Suggested change
| /* File doesn't exist, use parent dentry's inode if available */ | |
| /* Use parent directory's inode since target doesn't exist */ |
Comment on lines
293
to
388
| pr_err("deepin_err_notify: Failed to put netlink header\n"); | ||
| error = -EMSGSIZE; |
There was a problem hiding this comment.
The error variable is set to -EMSGSIZE at line 350, but this assignment occurs after msg_head is NULL, which means genlmsg_put failed. The error should be printed before being assigned so the actual failure can be logged, or the error code should be checked/logged after assignment. Currently, the code doesn't use or log this error value immediately, which could make debugging more difficult.
Suggested change
| pr_err("deepin_err_notify: Failed to put netlink header\n"); | |
| error = -EMSGSIZE; | |
| error = -EMSGSIZE; | |
| pr_err("deepin_err_notify: Failed to put netlink header (error: %d)\n", error); |
logic for file system error notifications - Add the `deepin_fs_err_event_data` structure to unify error event parameters - Implement the `notify_fs_error` function to support sending multiple filenames and related metadata via netlink - Improve the `prepare_and_notify_fs_error` function to support combining paths and filenames, and extracting inode information Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-p4
branch
from
d4844ec to
da94513
Compare
dongert
approved these changes
Dec 16, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dongert The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
deepin_fs_err_event_datastructure to unify errorevent parameters
notify_fs_errorfunction to support sendingmultiple filenames and related metadata via netlink
prepare_and_notify_fs_errorfunction to supportcombining paths and filenames, and extracting inode information
Signed-off-by: electricface [email protected]
Summary by Sourcery
Refactor Deepin filesystem error notification to build richer event data and send it through a unified netlink path.
New Features:
Enhancements: