Hi,
On 28/07/2023 10:45, Juergen Gross wrote:
On 28.07.23 11:38, Julien Grall wrote:
Hi Juergen,
On 28/07/2023 10:14, Juergen Gross wrote:
On 28.07.23 10:59, Julien Grall wrote:
Hi Juergen,
On 28/07/2023 07:23, Juergen Gross wrote:
On 27.07.23 23:53, Julien Grall wrote:
Hi Juergen,
On 24/07/2023 12:02, Juergen Gross wrote:
Struct xs_tdb_record_hdr is used for nodes stored in the data base.
When working on a node, struct node is being used, which is
including
the same information as struct xs_tdb_record_hdr, but in a different
format. Rework struct xs_tdb_record_hdr in order to prepare
including
it in struct node.
Do the following modifications:
- move its definition to xenstored_core.h, as the reason to put
it into
utils.h are no longer existing
- rename it to struct node_hdr, as the "tdb" in its name has only
historical reasons
- replace the empty permission array at the end with a comment about
the layout of data in the data base (concatenation of header,
permissions, node contents, and children list)
- use narrower types for num_perms and datalen, as those are
naturally
limited to XENSTORE_PAYLOAD_MAX (childlen is different here,
as it is
in theory basically unlimited)
The assumption is XENSTORE_PAYLOAD_MAX will never change and/or
always apply for all the connection. I am aware of at least one
downstream use where this is not the case.
I am happy to use narrower types, but I would at least like some
checks in Xenstore to ensure the limits are not reached.
I will add a BUILD_BUG_ON().
Initially I was thinking about a runtime check, but I am also fine
with a BUILD_BUG_ON() if it is right next to length check in
handle_input(). So if someone decided to add different payload size
depending on the domain (such as privileged domain could do more),
it would be easier to spot what else needs to be changed.
Is this really the correct placement?
I think so. By adding the BUILD_ON_BUG() right above the length check,
it would be easier for everyone to know that the datastructure may
also need to change. This ...
I've put it into write_node_raw(), as this is where the related
datalen member
is being filled.
... would be less obvious here and I think it might be miss.
Assuming that someone changing XENSTORE_PAYLOAD_MAX would do a build
afterwards,
I don't see how such a failure could be missed.
Because one may want dom0 to send payload bigger than
XENSTORE_PAYLOAD_MAX. Something like:
if ( conn->id != 0 && len < XENSTORE_PAYLOAD_MAX )
Such change would not be caught during compilation. AWS has a similar
check in the downstream tree because the implementation of
non-cooperative migration is using Xenstore command and we want to be
able to transfer the state in a single command.
In case of a runtime
check I
agree that a more central place would be preferred.
In the end I don't mind that much, but
BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
(typeof((struct node_hdr *)NULL->datalen))(-1));
is a little bit clumsy IMHO.
Agree. We could introduce FIELD_SIZEOF() (as Linux did) to hide the
complexity. The code would then look like:
>= (8 * FIELD_SIZEOF(struct node_hdr, datalen))
Cheers,
--
Julien Grall
