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

[Pratyush Yadav <pratyush@kernel.org>]

On Wed, Sep 23 2026, sashiko-bot@kernel.org wrote:

> 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
> --

Let me reply to Sashiko for this patch since I think we should take this
regardless of where this series goes. I think struct file is the wrong
primitive for liveupdate_get_token_outgoing().

I'll ignore Sashiko on other patches. It does raise some good points,
but code correctness isn't the main point of the series. I'll fix them
up on the main posting if we agree to go in this direction.

>
> commit 17b042e4901cfc25a86dec1038092fbe1eb5d060
> Author: Pratyush Yadav (Google) <pratyush@kernel.org>
>
> 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.

The IDs are supposed to be unique in the whole session. That's the whole
point of the ID mechanism. If handlers use overlapping IDs, the handlers
are broken and should be fixed.

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

This can be done but only when there is a need for it, which we have not
seen so far. Without a use-case, namespacing the IDs per-file-handler is
overengineering.

>
>>  {
>>  	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.

That's a broken caller and should be fixed.

>
> 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.

A very large number of files with dependencies need to be preserved for
this to be a problem. We haven't hit this bottleneck so far and don't
think will hit it at least in the near future.

>
> 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?

Not a bad idea to be fair. But that's an independent fixup.

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

-- 
Regards,
Pratyush Yadav