Re: [can-next] can: proc: remove pointers from CAN specific proc output

On 14.08.26 14:31, Sebastian Andrzej Siewior wrote:
> On 2026-08-14 12:54:38 [+0200], Oliver Hartkopp wrote:
> …
>> function names and sock inode numbers when available. As there's no known
>> tooling around the CAN specific proc output breaking the ABI with this
>> rework creates no issue.
> 
> I let you be the judge of that.
> 
> …
>> --- a/Documentation/networking/can.rst
>> +++ b/Documentation/networking/can.rst
>> @@ -1040,19 +1040,19 @@ 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::
>>   
>> -    foo@bar:~$ cat /proc/net/can/rcvlist_all
>> +    foo@bar:~$ cat /proc/net/can/rcvlist_fil
> 
> Is this _fil a typo?

No, it's a slightly different example.

>> -    receive list 'rx_all':
>> -      (vcan3: no entry)
>> -      (vcan2: no entry)
>> -      (vcan1: no entry)
>> -      device   can_id   can_mask  function  userdata   matches  ident
>> -       vcan0     000    00000000  f88e6370  f6c6f400         0  raw
>> +    receive list 'rx_fil':
>>         (any: no entry)
>> +      device   can_id   can_mask   matches     sock_inode     function
>> +       vcan0  80000123  c00007ff         0  000000000000f862  raw_rcv [can_raw]
>> +      (vcan1: no entry)
>> +      (vcan2: no entry)
>> +      (vcan3: no entry)
>>   
>>   In this example an application requests any CAN traffic from vcan0::
>>   
>>       rcvlist_all - list for unfiltered entries (no filter operations)
>>       rcvlist_eff - list for single extended frame (EFF) entries
> …
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -2039,11 +2038,11 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
>>   	}
>>   
>>   #if IS_ENABLED(CONFIG_PROC_FS)
>>   	if (net->can.bcmproc_dir) {
>>   		/* unique socket address as filename */
>> -		sprintf(bo->procname, "%llu", sock_i_ino(sk));
>> +		sprintf(bo->procname, "%016llx", sock_i_ino(sk));
> 
> snprintf() would be a bit bulletproof
> 

Will change that in v2

>>   		bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644,
>>   						     net->can.bcmproc_dir,
>>   						     bcm_proc_show, sk);
>>   		if (!bo->bcm_proc_read) {
>>   			ret = -ENOMEM;
> …
>> diff --git a/net/can/proc.c b/net/can/proc.c
>> index de4d05ae3459..b7858fb66178 100644
>> --- a/net/can/proc.c
>> +++ b/net/can/proc.c
>> @@ -189,30 +189,35 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>>   			      struct net_device *dev)
>>   {
>>   	struct receiver *r;
>>   
>>   	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";
>> +		char *fmt;
>>   
>> +#if IS_ENABLED(CONFIG_KALLSYMS)
> 
> Please don't. I fix %ps and then there is no leak. There will be then
> the 0 and everything will be fine.
> 

I can wait for your changes before the v2 posting.

Btw. do you think "0" is a good return value, when people expect a 
function name?

What about "(unknown)" or something similar?

>> +		fmt = (r->can_id & CAN_EFF_FLAG)?
>> +			"  %6s  %08x  %08x  %8ld  %016llx  %ps\n" :
>> +			"  %6s     %03x    %08x  %8ld  %016llx  %ps\n";
>>   		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, r->func);
>> +#else
>> +		fmt = (r->can_id & CAN_EFF_FLAG)?
>> +			"  %6s  %08x  %08x  %8ld  %016llx  (unknown)\n" :
>> +			"  %6s     %03x    %08x  %8ld  %016llx  (unknown)\n";
>> +		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
>> +			   atomic_long_read(&r->matches), r->ino);
>> +#endif
>>   	}
>>   }
> …
> 
> So if you do this now, then I probably should remove that hunk from my
> patch.

IMHO these /proc/net/can changes go far beyond replacing the pointer 
values with "0".

I hope the
Suggested-by: Sebastian Andrzej Siewior <[email protected]>
is ok for your attribution?!?

Best regards,
Oliver