[RFC PATCH v1 4/5] landlock: Add landlock_add_rule_fs tracepoint

[Tingmao Wang]

On 5/23/25 17:57, Mickaël Salaün wrote:
> Add a tracepoint for Landlock path_beneath rule addition.  This is
> useful to tie a Landlock object with a file for debug purpose.
> 
> Allocate the absolute path names when adding new rules.  This is OK
> because landlock_add_rule(2) is not a performance critical code.
> 
> Here is an example of landlock_add_rule_fs traces:
>    ruleset=0x000000007e3b1c4a key=inode:0xffff888004f59260 allowed=0xd dev=0:16 ino=306 path=/usr
>    ruleset=0x000000007e3b1c4a key=inode:0xffff888004f59240 allowed=0xffff dev=0:16 ino=346 path=/root
> 
> TODO: Use Landlock IDs instead of kernel addresses to identify Landlock
> objects (e.g. inode).

Do you mean like the u64 domain ID for audit, but for objects?  Since 
there currently isn't a u64, non-pointer ID, I guess we would have to 
create one for the object just for tracing?

My understanding is that kernel pointers are not hidden from the root 
user / someone who can read traces, so maybe just printing the pointer 
is good enough?

> 
> Cc: Günther Noack <gnoack at google.com>
> Cc: Masami Hiramatsu <mhiramat at kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> Cc: Steven Rostedt <rostedt at goodmis.org>
> Cc: Tingmao Wang <m at maowtm.org>
> Signed-off-by: Mickaël Salaün <mic at digikod.net>
> ---
>   MAINTAINERS                     |  1 +
>   include/trace/events/landlock.h | 68 +++++++++++++++++++++++++++++++++
>   security/landlock/Makefile      | 11 +++++-
>   security/landlock/fs.c          | 22 +++++++++++
>   security/landlock/fs.h          |  3 ++
>   security/landlock/trace.c       | 14 +++++++
>   6 files changed, 117 insertions(+), 2 deletions(-)
>   create mode 100644 include/trace/events/landlock.h
>   create mode 100644 security/landlock/trace.c
> 
 > > [...]
 > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 73a20a501c3c..e5d673240882 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -36,6 +36,7 @@
>   #include <linux/types.h>
>   #include <linux/wait_bit.h>
>   #include <linux/workqueue.h>
> +#include <trace/events/landlock.h>
>   #include <uapi/linux/fiemap.h>
>   #include <uapi/linux/landlock.h>
>   
> @@ -345,6 +346,27 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>   	mutex_lock(&ruleset->lock);
>   	err = landlock_insert_rule(ruleset, ref, access_rights);
>   	mutex_unlock(&ruleset->lock);
> +
> +	if (!err && trace_landlock_add_rule_fs_enabled()) {
> +		const char *pathname;
> +		/* Does not handle deleted files. */
> +		char *buffer __free(__putname) = __getname();
> +
> +		if (buffer) {
> +			const char *absolute_path =
> +				d_absolute_path(path, buffer, PATH_MAX);
> +			if (!IS_ERR_OR_NULL(absolute_path))
> +				pathname = absolute_path;
> +			else
> +				pathname = "<too_long>";

Not sure if it's necessary to go that far, but I think d_absolute_path 
returns -ENAMETOOLONG in the too long case, and -EINVAL in the "not 
possible to construct a path" case (I guess e.g. if it's an anonymous 
file or detached mount).  We could add an else if branch to check which 
case it is and use different strings.

> +		} else {
> +			/* Same format as audit_log_d_path(). */
> +			pathname = "<no_memory>";
> +		}
> +		trace_landlock_add_rule_fs(ruleset, &ref, access_rights, path,
> +					   pathname);
> +	}
> +
>   	/*
>   	 * No need to check for an error because landlock_insert_rule()
>   	 * increments the refcount for the new object if needed.
> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index bf9948941f2f..60be95ebfb0b 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -11,6 +11,7 @@
>   #define _SECURITY_LANDLOCK_FS_H
>   
>   #include <linux/build_bug.h>
> +#include <linux/cleanup.h>
>   #include <linux/fs.h>
>   #include <linux/init.h>
>   #include <linux/rcupdate.h>
> @@ -128,4 +129,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>   			    const struct path *const path,
>   			    access_mask_t access_hierarchy);
>   
> +DEFINE_FREE(__putname, char *, if (_T) __putname(_T))

Out of curiosity why not put this in include/linux/fs.h (seems to 
compile for me when added there)?

> +
>   #endif /* _SECURITY_LANDLOCK_FS_H */
> diff --git a/security/landlock/trace.c b/security/landlock/trace.c
> new file mode 100644
> index 000000000000..98874cda473b
> --- /dev/null
> +++ b/security/landlock/trace.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock - Tracepoints
> + *
> + * Copyright © 2025 Microsoft Corporation
> + */
> +
> +#include <linux/path.h>
> +
> +#include "access.h"
> +#include "ruleset.h"
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/landlock.h>