Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr

[Julien Grall <julien@xxxxxxx>]

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