Re: [PATCH RFC v2 1/2] tsm: Add TVM Measurement Register Support

["Xing, Cedric" <cedric.xing@intel.com>]

On 11/3/2024 9:51 PM, Alexey Kardashevskiy wrote:
> On 1/11/24 03:50, Cedric Xing wrote:
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm-core.c
>> similarity index 95%
>> rename from drivers/virt/coco/tsm.c
>> rename to drivers/virt/coco/tsm-core.c
>> index 9432d4e303f1..92e961f21507 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm-core.c
>> @@ -1,8 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /* Copyright(c) 2023 Intel Corporation. All rights reserved. */
>> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> -
> 
> Why remove it?
> 
It's not used anywhere...

>>   #include <linux/tsm.h>
>>   #include <linux/err.h>
>>   #include <linux/slab.h>
>> @@ -166,8 +164,9 @@ static ssize_t 
>> tsm_report_service_guid_store(struct config_item *cfg,
>>   }
>>   CONFIGFS_ATTR_WO(tsm_report_, service_guid);
>> -static ssize_t tsm_report_service_manifest_version_store(struct 
>> config_item *cfg,
>> -                             const char *buf, size_t len)
>> +static ssize_t
>> +tsm_report_service_manifest_version_store(struct config_item *cfg,
>> +                      const char *buf, size_t len)
> 
> Unrelated change usually goes to a separate preparation patch, otherwise 
> too much noise.
> 
You are right. I'll capture all the "noise" in a single preparation commit.

>> -MODULE_DESCRIPTION("Provide Trusted Security Module attestation 
>> reports via configfs");
>> +MODULE_DESCRIPTION(
>> +    "Provide Trusted Security Module attestation reports via configfs");
> 
> 
> Seems unrelated.
> 
Are you suggesting an edit to the module description or simply 
complaining about unrelated changes in the same commit?

>> diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
>> new file mode 100644
>> index 000000000000..a84e923a7782
>> --- /dev/null
>> +++ b/drivers/virt/coco/tsm-mr.c
>> @@ -0,0 +1,374 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
>> +
>> +#include <linux/tsm.h>
>> +#include <linux/shmem_fs.h>
>> +#include <linux/ctype.h>
>> +#include <crypto/hash_info.h>
>> +#include <crypto/hash.h>
>> +
>> +int tsm_mr_init(void);
>> +void tsm_mr_exit(void);
> 
> These two should go to drivers/virt/coco/tsm-mr.h, along with 
> tsm_measurement_register and other TSM_MR_F_*.
> 
TSM_MR_F_* are part of the module interface and have been defined in 
include/linux/tsm.h

These 2 are internal functions called by the module entry/exit points 
only. Their prototypes appear here merely to avoid the compiler warning.

>> +
>> +enum _mrdir_bin_attr_index {
> 
> Why do so many things have "_" prefix in this file?
> 
All "_" prefixed symbols are file local. I should have used a more 
explicit prefix. I'll change this in the next revision.

>> +    _MRDIR_BA_DIGEST,
>> +    _MRDIR_BA__COUNT,
> 
> One underscore would do.
> 
Are you talking about the double "__" in _MRDIR_BA__COUNT? It isn't part 
of the enum logically, so I put an extra "_". A precedence is 
include/uapi/linux/hash_info.h:41 in the existing kernel source.

>> [...]
>> +static void _mr_provider_release(struct kobject *kobj)
>> +{
>> +    struct _mr_provider *pvd;
>> +    pvd = container_of(kobj, typeof(*pvd), kset.kobj);
>> +    pr_debug("%s(%s)\n", __func__, kobject_name(kobj));
>> +    BUG_ON(!list_empty(&pvd->kset.list));
> 
> Harsh. These days people do not like even WARN_ON :) None of these 
> BUG_ONs seem bad enough to kill the system, dunno.
> 
This BUG_ON has helped me catch kobject leaks in my code. I don't have 
problem removing it. But is there a guideline on what kinds of 
BUG_ON/WARN_ON should be kept/removed?

>> [...]
>> +int tsm_register_measurement(struct tsm_measurement *tmr)
>> +{
>> +    static struct kobj_attribute _attr_hash = __ATTR_RO(hash_algo);
>> +
> 
> Extra empty line not needed.
> 
>> [...]
>> +        for (int j = 0; j < _MRDIR_BA__COUNT; ++j)
>> +            battrs[j] = &mrd->battrs[j];
> 
> An empty line missing here.
> 
Thanks for pointing these out!

>> [...]
>> +    pvd = NULL;
> 
> Is this needed for __free() machinery?
> 
Yes. I should have put a comment here.

>> [...]
>> +    struct kobject *kobj = kset_find_obj(_sysfs_tsm, tmr->name);
> 
> Empty line missing. scripts/checkpatch.pl should have detected it. Thanks,
> 
Will run scripts/checkpatch.pl on the next revision before sending it out.