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

[Alexey Kardashevskiy]

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