linux-kernel - Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <77dd25e4-a807-4a7f-902a-7b362f2ce6c0@ti.com>
Date: Mon, 2 Jun 2025 14:30:20 +0530
From: Beleswar Prasad Padhi <b-padhi@...com>
To: Dawei Li <dawei.li@...ux.dev>
CC: <andersson@...nel.org>, <mathieu.poirier@...aro.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <set_pte_at@...look.com>
Subject: Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL
 uAPI

Hi Dawei,

On 30/05/25 18:20, Dawei Li wrote:
> 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
>  - ...


Thank you for the extensive list of examples!

>
>>> - 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?


Perhaps yes. It makes sense to me now. Thanks for addressing my queries!

>
>>> - 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

>
>> 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