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

[Xing, Cedric]

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.