Re: [RFC PATCH 1/6] liveupdate: luo_file: look up outgoing tokens by id

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] liveupdate: NULL pointer dereference in liveupdate_get_token_outgoing() on incoming sessions
- [Medium] liveupdate: flat ID namespace in liveupdate_get_token_outgoing() risks cross-handler collisions
- [Low] liveupdate: O(N) indirect function calls in liveupdate_get_token_outgoing() list traversal
--

commit 17b042e4901cfc25a86dec1038092fbe1eb5d060
Author: Pratyush Yadav (Google) <pratyush@xxxxxxxxxx>

liveupdate: luo_file: look up outgoing tokens by id

liveupdate_get_token_outgoing() is used to look up tokens of dependencies.
It does the search based on struct file. LUO provides file handlers a
get_id callback that can return the unique identifier of a resource. This
patch uses ID to look up dependencies in liveupdate_get_token_outgoing()
instead of struct file.

> diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
> index a39992da504fb..51817d202ce26 100644
> --- a/kernel/liveupdate/luo_file.c
> +++ b/kernel/liveupdate/luo_file.c
[ ... ]
>  int liveupdate_get_token_outgoing(struct liveupdate_session *s,
> -				  struct file *file, u64 *tokenp)
> +				  unsigned long id, u64 *tokenp)

[Severity: Medium]
Will this flat ID namespace risk cross-handler collisions?

Because liveupdate_get_token_outgoing() searches across all preserved files
using just an unsigned long id, it loses the context of which file handler
the ID belongs to.

If different handlers use overlapping namespaces, such as IDR indices or
pointers that might alias depending on object lifetime, a lookup intended
for one handler's object could incorrectly match an object from a different
handler that happens to appear earlier in the list.

Could the API require the expected file handler context to scope the search
safely?

>  {
>  	struct luo_file_set *file_set = luo_file_set_from_session_locked(s);
>  	struct luo_file *luo_file;
>  	int err = -ENOENT;
>  
>  	list_for_each_entry(luo_file, &file_set->files_list, list) {
> -		if (luo_file->file == file) {
> +		if (luo_get_id(luo_file->fh, luo_file->file) == id) {

[Severity: High]
Can this cause a NULL pointer dereference on incoming sessions?

In an incoming session after deserialization, luo_file->file is initialized
to NULL and remains NULL until the file is explicitly retrieved.

Because liveupdate_get_token_outgoing() is an exported API that accepts a
generic struct liveupdate_session, an in-kernel caller could mistakenly pass
an incoming session.

Prior to this patch, doing so would safely return -ENOENT because NULL == file
is false. With this patch, luo_get_id() will blindly pass the NULL file
pointer to the handler's get_id callback.

Handlers like memfd_luo call file_inode(file), which would cause an
immediate NULL pointer dereference and kernel crash.

[Severity: Low]
Are these indirect function calls necessary during the list traversal?

Replacing the direct pointer comparison with luo_get_id() performs an indirect
function call (fh->ops->get_id) if defined, on every iteration of the
files_list traversal.

Because this function is used to look up tokens for dependencies, it can be
called for every dependency of every preserved object, resulting in O(N * M)
indirect calls.

Since the ID is a constant property of the preserved file, could it be
evaluated exactly once during luo_preserve_file() and cached directly in
struct luo_file?

>  			if (tokenp)
>  				*tokenp = luo_file->token;
>  			err = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923224408.3745689-1-pratyush@xxxxxxxxxx?part=1