Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li <dawei.li@linux.dev>]

HI Beleswar,

Thanks for reviewing.

On Fri, May 30, 2025 at 03:15:28PM +0530, Beleswar Prasad Padhi wrote:
> Hi Dawei,
> 
> On 19/05/25 20:38, Dawei Li wrote:
> > Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
> > shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
> > returns fd representing eptdev to userspace directly.
> >
> > Possible calling procedures for userspace are:
> > - fd = open("/dev/rpmsg_ctrlX")
> > - ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
> > - fd_ep = info.fd
> 
> 
> We are returning a new fd to userspace from inside an IOCTL itself. Is this a
> standard way of doing things in Kernel space? (see below related comment)

Yes, anon_get_{fd,file} are used extensively in kernel for returning a new
fd to userspace which is associated with an unique data structure in kernel
space, in different ways:

- via ioctl(), some examples are:

 - KVM ioctl(s)
   - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
   - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
   - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
   - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm 

 - DMA buf/fence/sync ioctls
   - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
   - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
   - Couples of driver implement DMA buf by using anon file _implicitly_:
     - UDMABUF_CREATE -> udmabuf_ioctl_create
     - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate

 - gpiolib ioctls:
   - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
   - GPIO_V2_GET_LINE_IOCTL

 -  IOMMUFD ioctls:

 -  VFIO Ioctls:

 - ....


- via other specific syscalls:
 - epoll_create1
 - bpf 
 - perf_event_open
 - inotify_init
 - ...

> 
> > - operations on fd_ep(write, read, poll ioctl)
> > - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> > - close(fd_ep)
> 
> 
> Can we rely on the userspace to close() the fd_ep? (if not done, could be a
> memory leak..).. Opposed to fd, which we can rely on the userspace to
> close() since they initiated the open() call. I am just trying to understand if
> this is a standard way of doing things...

Good question.

When userland gets a fd from kernel, it's userland's duty to manage and release
the resource when it's done with it, because kernel never knows when the fd and
its resourcen are not needed by userland except process is on exiting. The fact
remains true no matter how fd is generated from kernel:
- open()
- ioctl()
- Other syscalls(epoll_create1, e.g, as listed above)

As a result, kernel & driver provide fops->release() to achieve resource
release when fd is not needed for userland, some callers of it maybe:
- Userland call close() explicitly
- Kernel does the dirty job when user process exits(if some fds are
  still opened):
  - Userland call exit() explicitly.
  - User process was killed by some signals.

Maybe some comments/docs are needed in uAPI?

> 
> > - close(fd)
> >

[snip]

> > +
> > +	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
> > +	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
> > +		if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> > +			return -EFAULT;
> > +
> > +		memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> > +		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> > +		chinfo.src = eptinfo.src;
> > +		chinfo.dst = eptinfo.dst;
> > +	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
> 
> 
> Maybe we can put this 'else if condition' in the first 'if' and treat other
> conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
> ioctl with a different struct type.

Good point! I will try to address it in next respin.

> 
> Thanks,
> Beleswar
> 
> > +		if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
> > +			return -EFAULT;
> > +
> > +		memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
> > +		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> > +		chinfo.src = ept_fd_info.src;
> > +		chinfo.dst = ept_fd_info.dst;
> > +	}
> >  

[snip]

Thanks,

	Dawei