Re: [PATCH v4 hurd] pci: Add RPC for grabbing io ports by region

[Sergey Bugaev]

Hello,

as said before, I can only nitpick on the implementation details, hope
this is still useful :)

On Sun, Jul 23, 2023 at 6:36 AM Damien Zammit <damien@zamaudio.com> wrote:
> +
> +/*
> + * Request io ports for a specific io region for a device.
> + * The resulting port may be used to call mach rpc i386_io_perm_modify.
> + * Only works on io region files.
> + */
> +routine pci_request_io_ports(
> +       master: pci_t;
> +       out io_perm: mach_port_t
> +);

io_perm: io_perm_t rather?

> +error_t
> +request_io_ports (struct pcifs_dirent *e, io_perm_t *io_perm)
> +{
> +  int bar;
> +  uint64_t iobase, iosize;
> +  io_port_t from, to;
> +  error_t err;
> +  mach_port_t device_master;
> +
> +  if (get_privileged_ports (0, &device_master))
> +    return EPERM;

Perhaps try to propagate the error get_privileged_ports () returns?

> +
> +  bar = strtol (&e->name[strlen (FILE_IO_NAME)], NULL, 10);
> +  if (errno)
> +    return errno;
> +
> +  if ((bar < 0) || (bar > 5))
> +    /* This operation only works on IO bars between 0-5 */
> +    return EINVAL;
> +
> +  iobase = e->device->regions[bar].base_addr;
> +  iosize = e->device->regions[bar].size;
> +  if ((iobase >= 0x10000) || (iosize >= 0x10000) || (iobase + iosize >= 
> 0x10000))
> +    return EINVAL;

In all of these early return cases, you're leaking the device_master
reference that get_privileged_ports () returns. You should just do
this validation first & get the port after them.

> +
> +  from = iobase;
> +  to = from + iosize - 1;
> +
> +  if ((err = i386_io_perm_create (device_master, from, to, io_perm)))
> +    return err;

Same here, if this fails you need to deallocate the device master. You
need to deallocate it on the success path too, so it's easiest to do:

err = i386_io_perm_create (device_master, from, to, io_perm);
if (!err)
  UPDATE_TIMES (e, TOUCH_ATIME);
mach_port_deallocate (mach_task_self (), device_master);
return err;

Yes, this doesn't check the return value of mach_port_deallocate ().
It's awkward to, unless you want to introduce err2, so for cases like
this (where you care about the err value) it's OK to skip checking.

> +
> +  /* Update atime */
> +  UPDATE_TIMES (e, TOUCH_ATIME);
> +
> +  return 0;
> +}
> +
> +error_t
> +S_pci_request_io_ports (struct protid * master, io_perm_t *io_perm)

struct protid *master

and actually I see there's quite a lot of this style (type * name) in
pci-arbiter; is that intended?

Sergey