Re: [PATCH v7 16/23] firmware: arm_scmi: Add Telemetry debugfs SHMTI dump support

Fayssal Benmlih <[email protected]>
Newsgroups gmane.linux.kernel,gmane.linux.ports.arm.kernel,gmane.linux.documentation
Message-ID <[email protected]>
Hi Cristian,

Two debugfs issues inline.

> 	shmti = inode->i_private;
> 	sblob = kzalloc_obj(*sblob);
> 	if (!sblob)
> 		return -ENOMEM;
>
> 	sblob->shmti = shmti;

An open debugfs file can outlive debugfs_remove_recursive() and protocol
removal. sblob retains a pointer to the devm-owned telemetry_shmti
descriptor and its devm mapping, but open does not pin the SCMI
device/protocol or coordinate with debugfs removal.

A later read from an already open fd can therefore dereference released
SHMTI state.

Please use the debugfs file-lifetime helpers or otherwise pin the owning
device/protocol from open until release and prevent reads once teardown
begins.

> 	for (unsigned int i = 0; i < ti->num_shmti; i++) {
> 		char id[16];
>
> 		snprintf(id, 16, "%u", i);
> 		debugfs_create_file_size(id, 0444, shmti_top,
> 					 &ti->shmti[i],
> 					 &scmi_telemetry_dbg_shmti_fops,
> 					 ti->shmti[i].info.len);
> 	}

The file is named using the array index, but the ABI documentation says it
is named using the SHMTI ID. Firmware-provided SHMTI IDs are not
necessarily identical to enumeration indexes.

Please either use ti->shmti[i].info.sid as the filename or document the
filename as an enumeration index.

Thanks,
Fayçal
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.