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

[Alexey Kardashevskiy <aik@amd.com>]

On 5/11/24 09:14, Xing, Cedric wrote:
> 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.


I am not even sure we want necessarily all of this changed, we can have 
100 char lines now.

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

The unrelated change complain.

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

I do not think you need any prefix here (just mark those "static"), and 
"mr" is there already anyway, imho more than enough.


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

okay :)

>>> [...]
>>> +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?

Sounds like only hardware faults are okay-ish to be guarded with BUG_ON.

https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on

WARN_ON is considered as bad as many use panic_on_oops. Thanks,


>>> [...]
>>> +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.

-- 
Alexey