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

[Juergen Gross <jgross@xxxxxxxx>]

On 28.07.23 12:34, Julien Grall wrote:
> 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.
And how directly is this related to the max data size of node contents?

As soon as you are allowing dom0 to write larger nodes, you are risking to
kill client connections trying to read such a node. So the node size should
still be limited to XENSTORE_PAYLOAD_MAX.

IMO another reason to use the placement I've suggested. AWS should even add
a size check when writing nodes to make sure dom0 doesn't do evil things.

> > 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))
Oh, I guess you mean sizeof_field().

And even with that it would look quite clumsy:

        BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
                     (1UL << (8 * sizeof_field(struct node_hdr, datalen))));


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature