Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities

[Julien Grall <julien@xxxxxxx>]

Hi Vikram,

A few more comments that I spotted after reviewing the next patch.

On 08/03/2022 19:47, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
> device tree overlay.
>
> xl overlay remove file.dtbo:
>      Removes all the nodes in a given dtbo.
>      First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
>      in dt_host and delete the device node entries from dt_host.
>
>      The nodes get removed only if it is not used by any of dom0 or domio.
Can overlay be nested (let say B nest in A)? If yes, how do you deal 
with the case the A is removed before B?
[...]

> +long dt_sysctl(struct xen_sysctl *op)
> +{
> +    long ret = 0;
> +    void *overlay_fdt;
> +    char **nodes_full_path = NULL;
> +    unsigned int num_overlay_nodes = 0;
> +
> +    if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
From my understanding, FDT are typically limited to 2MB. At minimum, we 
should check the overlay is not bigger than that (to avoid arbirtrary 
allocation size). I would possibly consider to limit to lower than that 
(i.e 500KB) if there is no need to have larger and to reduce the amount 
memory consumption by the overlay code.
> +        return -EINVAL;
> +
> +    overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
> +
> +    if ( overlay_fdt == NULL )
> +        return -ENOMEM;
> +
> +    ret = copy_from_guest(overlay_fdt, op->u.dt_overlay.overlay_fdt,
> +                         op->u.dt_overlay.overlay_fdt_size);
> +    if ( ret )
> +    {
> +        gprintk(XENLOG_ERR, "copy from guest failed\n");
> +        xfree(overlay_fdt);
You free overlay_fdt, but not in the other paths.

> +
> +        return -EFAULT;
> +    }
> +
> +    switch ( op->u.dt_overlay.overlay_op )
> +    {
> +    case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> +        ret = check_overlay_fdt(overlay_fdt,
> +                                op->u.dt_overlay.overlay_fdt_size);
> +        if ( ret )
> +        {
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        num_overlay_nodes = overlay_node_count(overlay_fdt);
> +        if ( num_overlay_nodes == 0 )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        ret = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> +                                     num_overlay_nodes);
> +        if ( ret )
> +             break;
> +
> +        ret = handle_remove_overlay_nodes(nodes_full_path,
> +                                          num_overlay_nodes);
> +        break;
> +
> +    default:
> +        break;
> +    }
> +
> +    if ( nodes_full_path != NULL )
> +    {
> +        int i;
> +        for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
> +        {
> +            xfree(nodes_full_path[i]);
> +        }
> +        xfree(nodes_full_path);
> +    }
AFAICT, nodes_full_path is not going to be used by the subop to add an 
overlay. So I would consider to move this within the case or (even 
better) create a function handling the subop (like you did for add) so 
we don't end up with a large switch.
Cheers,

--
Julien Grall