linux-next: build failure after merge of the devsec-tsm tree

Posted by Stephen Rothwell 7 months, 1 week ago

Hi all,

After merging the devsec-tsm tree, today's linux-next build (x86_64
allmodconfig) failed like this:

drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
  228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
      |                             ^

Caused by commit

  29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")

interacting with commit

  9bec944506fa ("sysfs: constify attribute_group::bin_attrs")

from the driver-core tree.

I have applied the following merge resolution for today (there must be
a better solution).

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 8 May 2025 17:52:55 +1000
Subject: [PATCH] fix up for "tsm-mr: Add TVM Measurement Register support"

interacting with "sysfs: constify attribute_group::bin_attrs" from the
driver-core tree.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/virt/coco/guest/tsm-mr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virt/coco/guest/tsm-mr.c b/drivers/virt/coco/guest/tsm-mr.c
index d75b08548292..37e6650bfb6c 100644
--- a/drivers/virt/coco/guest/tsm-mr.c
+++ b/drivers/virt/coco/guest/tsm-mr.c
@@ -225,7 +225,7 @@ tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
 
 	init_rwsem(&ctx->rwsem);
 	ctx->agrp.name = "measurements";
-	ctx->agrp.bin_attrs = no_free_ptr(bas);
+	ctx->agrp.bin_attrs = (const struct bin_attribute *const *)no_free_ptr(bas);
 	ctx->tm = tm;
 	return &no_free_ptr(ctx)->agrp;
 }
-- 
2.47.2

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Dan Williams 7 months, 1 week ago

Stephen Rothwell wrote:
> Hi all,
> 
> After merging the devsec-tsm tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
> 
> drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
> drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
>   228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
>       |                             ^
> 
> Caused by commit
> 
>   29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")
> 
> interacting with commit
> 
>   9bec944506fa ("sysfs: constify attribute_group::bin_attrs")
> 
> from the driver-core tree.
> 
> I have applied the following merge resolution for today (there must be
> a better solution).

Indeed.

So it looks like while there are plenty of dynamic binary attribute
creation users (see sysfs_bin_attr_init() callers). There are zero that
attempt to assign dynamically allocated attributes to be registered by a
static @groups.

The @groups publishing model is preferable because the lifetime rules
are all handled by the driver core at device add/del time.

So, while there is still casting involved, I think a better solution is
to make the allocation const and then cast for init ala incremental
patch below. Cedric, if this looks ok to you I'll send out another
partial-reroll to get this fixed up so the build breakage stays out of
bisection runs.

-- 8< --
diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
index d75b08548292..7fe90fae2738 100644
--- a/drivers/virt/coco/tsm-mr.c
+++ b/drivers/virt/coco/tsm-mr.c
@@ -169,24 +169,27 @@ tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
 	}
 
 	/*
-	 * @bas and the MR name strings are combined into a single allocation
+	 * @attrs and the MR name strings are combined into a single allocation
 	 * so that we don't have to free MR names one-by-one in
 	 * tsm_mr_free_attribute_group()
 	 */
-	struct bin_attribute **bas __free(kfree) =
-		kzalloc(sizeof(*bas) * (tm->nr_mrs + 1) + nlen, GFP_KERNEL);
+	const struct bin_attribute * const *attrs __free(kfree) =
+		kzalloc(sizeof(*attrs) * (tm->nr_mrs + 1) + nlen, GFP_KERNEL);
 	struct tm_context *ctx __free(kfree) =
 		kzalloc(struct_size(ctx, mrs, tm->nr_mrs), GFP_KERNEL);
 	char *name, *end;
 
-	if (!ctx || !bas)
+	if (!ctx || !attrs)
 		return ERR_PTR(-ENOMEM);
 
-	/* @bas is followed immediately by MR name strings */
-	name = (char *)&bas[tm->nr_mrs + 1];
+	/* @attrs is followed immediately by MR name strings */
+	name = (char *)&attrs[tm->nr_mrs + 1];
 	end = name + nlen;
 
 	for (size_t i = 0; i < tm->nr_mrs; ++i) {
+		/* break const for init */
+		struct bin_attribute **bas = (struct bin_attribute **)attrs;
+
 		bas[i] = &ctx->mrs[i];
 		sysfs_bin_attr_init(bas[i]);
 
@@ -225,7 +228,7 @@ tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
 
 	init_rwsem(&ctx->rwsem);
 	ctx->agrp.name = "measurements";
-	ctx->agrp.bin_attrs = no_free_ptr(bas);
+	ctx->agrp.bin_attrs = no_free_ptr(attrs);
 	ctx->tm = tm;
 	return &no_free_ptr(ctx)->agrp;
 }

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Thomas Weißschuh 7 months, 1 week ago

Hi Dan,

On 2025-05-08 17:37:41-0700, Dan Williams wrote:
> Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the devsec-tsm tree, today's linux-next build (x86_64
> > allmodconfig) failed like this:
> > 
> > drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
> > drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
> >   228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
> >       |                             ^
> > 
> > Caused by commit
> > 
> >   29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")
> > 
> > interacting with commit
> > 
> >   9bec944506fa ("sysfs: constify attribute_group::bin_attrs")
> > 
> > from the driver-core tree.
> > 
> > I have applied the following merge resolution for today (there must be
> > a better solution).
> 
> Indeed.
> 
> So it looks like while there are plenty of dynamic binary attribute
> creation users (see sysfs_bin_attr_init() callers). There are zero that
> attempt to assign dynamically allocated attributes to be registered by a
> static @groups.
>
> The @groups publishing model is preferable because the lifetime rules
> are all handled by the driver core at device add/del time.
> 
> So, while there is still casting involved, I think a better solution is
> to make the allocation const and then cast for init ala incremental
> patch below. Cedric, if this looks ok to you I'll send out another
> partial-reroll to get this fixed up so the build breakage stays out of
> bisection runs.

Take a look at nvmem_populate_sysfs_cells() in drivers/nvmem/core.c.
It uses an intermediary non-const pointer for the initialization and
works without any casts.

Could you then also use .bin_attrs_new?
.read_new and .write_new are already used.


Thomas

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Dan Williams 7 months ago

Thomas Weißschuh wrote:
> Hi Dan,
> 
> On 2025-05-08 17:37:41-0700, Dan Williams wrote:
> > Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > After merging the devsec-tsm tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> > > 
> > > drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
> > > drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
> > >   228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
> > >       |                             ^
> > > 
> > > Caused by commit
> > > 
> > >   29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")
> > > 
> > > interacting with commit
> > > 
> > >   9bec944506fa ("sysfs: constify attribute_group::bin_attrs")
> > > 
> > > from the driver-core tree.
> > > 
> > > I have applied the following merge resolution for today (there must be
> > > a better solution).
> > 
> > Indeed.
> > 
> > So it looks like while there are plenty of dynamic binary attribute
> > creation users (see sysfs_bin_attr_init() callers). There are zero that
> > attempt to assign dynamically allocated attributes to be registered by a
> > static @groups.
> >
> > The @groups publishing model is preferable because the lifetime rules
> > are all handled by the driver core at device add/del time.
> > 
> > So, while there is still casting involved, I think a better solution is
> > to make the allocation const and then cast for init ala incremental
> > patch below. Cedric, if this looks ok to you I'll send out another
> > partial-reroll to get this fixed up so the build breakage stays out of
> > bisection runs.
> 
> Take a look at nvmem_populate_sysfs_cells() in drivers/nvmem/core.c.
> It uses an intermediary non-const pointer for the initialization and
> works without any casts.
> 
> Could you then also use .bin_attrs_new?
> .read_new and .write_new are already used.

Looks good, @Cedric, can you send an incremental cleanup to remove the
casts on top of commit:

7c3f259dfe03 virt: tdx-guest: Transition to scoped_cond_guard for mutex operations

...from tsm.git#next?

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Greg KH 7 months, 1 week ago

On Thu, May 08, 2025 at 05:37:41PM -0700, Dan Williams wrote:
> Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the devsec-tsm tree, today's linux-next build (x86_64
> > allmodconfig) failed like this:
> > 
> > drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
> > drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
> >   228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
> >       |                             ^
> > 
> > Caused by commit
> > 
> >   29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")
> > 
> > interacting with commit
> > 
> >   9bec944506fa ("sysfs: constify attribute_group::bin_attrs")
> > 
> > from the driver-core tree.
> > 
> > I have applied the following merge resolution for today (there must be
> > a better solution).
> 
> Indeed.
> 
> So it looks like while there are plenty of dynamic binary attribute
> creation users (see sysfs_bin_attr_init() callers). There are zero that
> attempt to assign dynamically allocated attributes to be registered by a
> static @groups.
> 
> The @groups publishing model is preferable because the lifetime rules
> are all handled by the driver core at device add/del time.
> 
> So, while there is still casting involved, I think a better solution is
> to make the allocation const and then cast for init ala incremental
> patch below. Cedric, if this looks ok to you I'll send out another
> partial-reroll to get this fixed up so the build breakage stays out of
> bisection runs.

Ick, yeah, that seems ok.

But what are these binary files for?  I looked in the documentation and
found this entry:
	/sys/devices/virtual/misc/tdx_guest/measurements/rtmr[0123]:sha384
is that these binary files?

Why is sysfs being used to expose binary "registers" and not done
through the ioctl api instead?  That's an internal kernel-computed
structure, not coming from the hardware, or am I mistaken?

thanks,

greg k-h

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Dan Williams 7 months ago

Greg KH wrote:
> On Thu, May 08, 2025 at 05:37:41PM -0700, Dan Williams wrote:
> > Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > After merging the devsec-tsm tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> > > 
> > > drivers/virt/coco/guest/tsm-mr.c: In function 'tsm_mr_create_attribute_group':
> > > drivers/virt/coco/guest/tsm-mr.c:228:29: error: assignment to 'const struct bin_attribute * const*' from incompatible pointer type 'struct bin_attribute **' [-Wincompatible-pointer-types]
> > >   228 |         ctx->agrp.bin_attrs = no_free_ptr(bas);
> > >       |                             ^
> > > 
> > > Caused by commit
> > > 
> > >   29b07a7b8f41 ("tsm-mr: Add TVM Measurement Register support")
> > > 
> > > interacting with commit
> > > 
> > >   9bec944506fa ("sysfs: constify attribute_group::bin_attrs")
> > > 
> > > from the driver-core tree.
> > > 
> > > I have applied the following merge resolution for today (there must be
> > > a better solution).
> > 
> > Indeed.
> > 
> > So it looks like while there are plenty of dynamic binary attribute
> > creation users (see sysfs_bin_attr_init() callers). There are zero that
> > attempt to assign dynamically allocated attributes to be registered by a
> > static @groups.
> > 
> > The @groups publishing model is preferable because the lifetime rules
> > are all handled by the driver core at device add/del time.
> > 
> > So, while there is still casting involved, I think a better solution is
> > to make the allocation const and then cast for init ala incremental
> > patch below. Cedric, if this looks ok to you I'll send out another
> > partial-reroll to get this fixed up so the build breakage stays out of
> > bisection runs.
> 
> Ick, yeah, that seems ok.
> 
> But what are these binary files for?  I looked in the documentation and
> found this entry:
> 	/sys/devices/virtual/misc/tdx_guest/measurements/rtmr[0123]:sha384
> is that these binary files?

Yes, and the expectation is that other confidential archs (ARM CCA and
RISCV COVE) will also publish a "measurements" group.

> Why is sysfs being used to expose binary "registers" and not done
> through the ioctl api instead?  That's an internal kernel-computed
> structure, not coming from the hardware, or am I mistaken?

Cedric clarified that it is coming from hardware/firmware, but I will
also note this file interface is in the same class as
"/sys/class/tpm/tpmX/pcr-<H>/<N>" and
"/sys/kernel/config/tsm/report/$name/outblob". I.e. platform provided
attestation evidence to be shipped to a verifier.

Re: linux-next: build failure after merge of the devsec-tsm tree

Posted by Xing, Cedric 7 months ago

On 5/9/2025 2:12 AM, Greg KH wrote:
[...]
> But what are these binary files for?  I looked in the documentation and
> found this entry:
> 	/sys/devices/virtual/misc/tdx_guest/measurements/rtmr[0123]:sha384
> is that these binary files?
> 
All files (including rtmr[0123]:sha384) under 
/sys/devices/virtual/misc/tdx_guest/measurements/ are TDX measurement 
registers, one file (sysfs binary attribute) per each register.

> Why is sysfs being used to expose binary "registers" and not done
> through the ioctl api instead?
Sysfs is preferred over ioctl for exposing TD measurement registers for 
several reasons:

- Global Register Values: The register values are global and not tied to 
specific file descriptors of the tdx_guest device.
- Intuitive Operations: The operations supported by these registers can 
be intuitively mapped to file read/write operations.
- Ease of Access: Sysfs attributes allow easy enumeration and access 
from all programming languages, including shell commands and scripts. 
This ease of access is beneficial for application debugging, enabling, 
and platform diagnosis/maintenance, as these measurements are relevant 
to all SW running inside the same TD.

> That's an internal kernel-computed
> structure, not coming from the hardware, or am I mistaken?
> 
These are measurement registers of the current TD on Intel platforms. 
They are read together via the TDG.MR.REPORT TDCALL then broken down 
into individual register values. They are NOT computed by the kernel but 
come directly from the TDX ISA.