Re: [PATCH v2 03/29] tools/xenlogd: connect to frontend

[Juergen Gross <jgross@xxxxxxxx>]

On 20.11.23 16:49, Jason Andryuk wrote:
> On Fri, Nov 10, 2023 at 1:04 PM Juergen Gross <jgross@xxxxxxxx> wrote:
> > Add the code for connecting to frontends to xenlogd.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > diff --git a/tools/xen-9pfsd/xen-9pfsd.c b/tools/xen-9pfsd/xen-9pfsd.c
> > index c365b35fe5..cc5734402d 100644
> > --- a/tools/xen-9pfsd/xen-9pfsd.c
> > +++ b/tools/xen-9pfsd/xen-9pfsd.c
> > +static int check_host_path(device *device)
> > +{
> > +    struct stat statbuf;
> > +    char *path, *p;
> > +    int ret = 1;
> > +
> > +    if ( !device->host_path )
> > +        return 1;
> > +
> > +    if ( device->host_path[0] != '/' )
> > +        return 1;
> > +
>  From v1, you stated for alloc_fid_mem(device, fid, path):
> > No, "path" is always starting with a "/" if it is not empty.
> And then
> snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);
>
> While alloc_fid_mem() uses "%s%s"
>
> And p9_create() uses "%s/%s"
Of course it does, as this is the concatenation of the current path with
the new file name, which is relative to the current path.

> p9_walk does:
> const char *rel_path = path + strlen(device->host_path)
> ...
> alloc_fid_mem(device, fid, rel_path);
>
> So host_path is expected not to have a tailing '/' to ensure that
> rel_path starts with a '/'.  So you want to error out for a trailing
> '/' (or overwrite with '\0')?
I can add that.

> It seems like alloc_fid_mem() should also check to ensure path is "'/'
> if it is not empty".
I'll add an assert().

> This is all subtle and security relevant, so it's important to get
> this right.  A code comment explaining the expectation of paths for
> host_path vs. fids would be good.
Agreed.

> Also, maybe using openat() would be a better approach?  Create the
> dirfd pointing at the 9pfs export and then use relative paths for the
> paths inside.  This would cut down on the manual path manipulations.
I'll look into that. I'll have to trade special casing accessing the root
directory vs. reducing path operations.

> > +    path = strdup(device->host_path);
> > +    if ( !path )
> > +    {
> > +        syslog(LOG_CRIT, "memory allocation failure!");
> > +        return 1;
> > +    }
> > +
> > +    for ( p = path; p; )
> > +    {
> > +        p = strchr(p + 1, '/');
> > +        if ( p )
> > +            *p = 0;
> > +        if ( !stat(path, &statbuf) )
> > +        {
> > +            if ( !(statbuf.st_mode & S_IFDIR) )
> > +                break;
> > +            if ( !p )
> > +            {
> > +                ret = 0;
> > +                break;
> > +            }
> > +            *p = '/';
> > +            continue;
> > +        }
> > +        if ( mkdir(path, 0777) )
> > +            break;
> > +        if ( p )
> > +            *p = '/';
> > +    }
> > +
> > +    free(path);
> > +    return ret;
> > +}
> > +
> > +
> > +static int write_backend_node(device *device, const char *node, const char 
> > *val)
> > +{
> > +    struct path p;
> > +    struct xs_permissions perms[2] = {
> > +        { .id = 0, .perms = XS_PERM_NONE },
> This hard codes dom0.  If xs_permissions supported DOMID_SELF, it
> wouldn't need to be looked up.
DOMID_SELF isn't supported. But you are right, I need to avoid hard coding dom0
here. This can be achieved by reading the permissions after creating the node
and use the result for the first permission entry.

> > +        { .id = device->domid, .perms = XS_PERM_READ }
> > +    };
> > +
> > +    construct_backend_path(device, node, &p);
> > +    if ( !xs_write(xs, XBT_NULL, p.path, val, strlen(val)) )
> > +    {
> > +        syslog(LOG_ERR, "error writing bacḱend node \"%s\" for device %u/%u",
> > +               node, device->domid, device->devid);
> > +        return 1;
> > +    }
> > +
> > +    if ( !xs_set_permissions(xs, XBT_NULL, p.path, perms, 2) )
> > +    {
> > +        syslog(LOG_ERR, "error setting permissions for \"%s\"", p.path);
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> > +static void connect_device(device *device)
> > +{
> > +    unsigned int val;
> > +    unsigned int ring_idx;
> > +    char node[20];
> > +    struct ring *ring;
> > +    xenevtchn_port_or_error_t evtchn;
> > +
> > +    val = read_frontend_node_uint(device, "version", 0);
> > +    if ( val != 1 )
> > +        return connect_err(device, "frontend specifies illegal version");
> > +    device->num_rings = read_frontend_node_uint(device, "num-rings", 0);
> > +    if ( device->num_rings < 1 || device->num_rings > MAX_RINGS )
> > +        return connect_err(device, "frontend specifies illegal ring number");
> > +
> > +    for ( ring_idx = 0; ring_idx < device->num_rings; ring_idx++ )
> > +    {
> > +        ring = calloc(1, sizeof(*ring));
> > +        if ( !ring )
> > +            return connect_err(device, "could not allocate ring memory");
> > +        device->ring[ring_idx] = ring;
> > +        ring->device = device;
> > +        pthread_cond_init(&ring->cond, NULL);
> > +        pthread_mutex_init(&ring->mutex, NULL);
> > +
> > +
> extra blank line.
Will remove it.


Thanks,

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature