Re: [PATCH net-next 05/37] can: proc: remove pointers from CAN specific proc output

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new CONFIG_KALLSYMS guard in can_print_rcvlist()
  (net/can/proc.c) does not stop the receiver callback address from…
- [Low] The updated text in Documentation/networking/can.rst says every
  rcvlist entry contains 'the CAN network device and the subscribed…

--

can: proc: remove pointers from CAN specific proc output

This patch stops printing kernel pointers in the /proc/net/can/ and
/proc/net/can-bcm/ output. The rcvlist entries now show the receiver
callback name via %ps (or "(unknown)" when CONFIG_KALLSYMS=n) and the
socket inode number. can_rx_register() now takes an inode number in
place of the ident string.

Link: https://patch.msgid.link/20261012115851.3412-1-socketcan@hartkopp.net

> diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
> index 536ff411da1d..908c199cf6ad 100644
> --- a/Documentation/networking/can.rst
> +++ b/Documentation/networking/can.rst
> @@ -1040,17 +1040,17 @@ As described in :ref:`socketcan-receive-lists` the SocketCAN core uses several f
>  lists to deliver received CAN frames to CAN protocol modules. These
>  receive lists, their filters and the count of filter matches can be
>  checked in the appropriate receive list. All entries contain the
> -device and a protocol module identifier::
> +CAN network device and the subscribed function name::

[Severity: Low]
Is this sentence accurate for every configuration?

With CONFIG_KALLSYMS=n, can_print_rcvlist() prints the literal string
"(unknown)" in place of the function name. The commit message mentions
this, but the documentation does not.

The new sock_inode column is also not explained. Kernel-internal
subscribers pass ino=0:

net/can/gw.c:cgw_register_filter()
    gwj, 0, NULL);

net/can/j1939/main.c:j1939_can_rx_register()
    j1939_can_recv, priv, 0, NULL);

Could the text mention the "(unknown)" case and say that a sock_inode of
0 means there is no associated socket?

>  
>      foo@bar:~$ cat /proc/net/can/rcvlist_all

[ ... ]

> diff --git a/net/can/proc.c b/net/can/proc.c
> index 64b3bdc2fa7e..64994ab48541 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -215,25 +215,27 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>  
>  	hlist_for_each_entry_rcu(r, rx_list, list) {
>  		char *fmt = (r->can_id & CAN_EFF_FLAG)?
> -			"   %-5s  %08x  %08x  %pK  %pK  %8ld  %s\n" :
> -			"   %-5s     %03x    %08x  %pK  %pK  %8ld  %s\n";
> +			"  %6s  %08x  %08x  %8ld  %10llu" :
> +			"  %6s     %03x    %08x  %8ld  %10llu";
>  
>  		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> -			   r->func, r->data, atomic_long_read(&r->matches),
> -			   r->ident);
> +			   atomic_long_read(&r->matches), r->ino);
> +
> +		/* without kallsyms %ps would print the raw text address */
> +		if (IS_ENABLED(CONFIG_KALLSYMS))
> +			seq_printf(m, "  %ps\n", r->func);

[Severity: Medium]
Can %ps still print the raw callback address when CONFIG_KALLSYMS=y?

The IS_ENABLED() check covers only the config option. It does not check
whether the symbol lookup succeeds. When kallsyms_lookup_buildid()
fails, the %ps path falls back to an unhashed address:

symbol_string()
  sprint_symbol_no_offset()
    __sprint_symbol()
      kallsyms_lookup_buildid()
        module_address_lookup()
          find_kallsyms_symbol()

kernel/module/kallsyms.c:find_kallsyms_symbol() {
    ...
	if (!best)
		return NULL;
    ...
}

kernel/kallsyms.c:__sprint_symbol() {
    ...
	if (!len)
		return sprintf(buffer, "0x%lx", address - symbol_offset);
    ...
}

The callbacks registered here are all static functions in modules:
raw_rcv, bcm_rx_handler, isotp_rcv, isotp_rcv_echo, can_can_gw_rcv and
j1939_can_recv.

scripts/Makefile.modinst passes custom INSTALL_MOD_STRIP values straight
to strip, so a module installed with something like
INSTALL_MOD_STRIP="--strip-unneeded" or "-x" can lose these local
symbols. If no named symbol comes before the callback in that module's
text, the lookup fails. This depends on section layout, which has not
been fully confirmed.

The rcvlist files are created 0644 in can_init_proc(), so any local user
can read them. The old %pK output applied kptr_restrict. In this
configuration the new output would show the real module text address.

Would it be safer to resolve the name explicitly, for example with
sprint_symbol_no_offset() or lookup_symbol_name(), check that the lookup
succeeded, and print "(unknown)" on any failure? The commit message says
the patch "removes the disclosure of pointers", so it seems worth closing
this case too.

> +		else
> +			seq_puts(m, "  (unknown)\n");
>  	}
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de