| Message ID | 12c806f9-f283-5bed-d137-7719ba73205a@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush > callback being called from multiple processes is an issue, maybe the > flush callback isn't appropriate after all. Userspace could also grab a reference just by opening /proc/$pid/fd/*. The idea is just that when any process which used the fd is killed by a signal we drop the remaining jobs from being submitted to the hardware. Christian.
On 2018-05-18 11:42 AM, Christian König wrote: > >> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush >> callback being called from multiple processes is an issue, maybe the >> flush callback isn't appropriate after all. > > Userspace could also grab a reference just by opening /proc/$pid/fd/*. > > The idea is just that when any process which used the fd is killed by a > signal we drop the remaining jobs from being submitted to the hardware. This must only affect jobs submitted by the killed process, not those submitted by other processes.
Am 18.05.2018 um 16:44 schrieb Michel Dänzer: > On 2018-05-18 11:42 AM, Christian König wrote: >>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush >>> callback being called from multiple processes is an issue, maybe the >>> flush callback isn't appropriate after all. >> Userspace could also grab a reference just by opening /proc/$pid/fd/*. >> >> The idea is just that when any process which used the fd is killed by a >> signal we drop the remaining jobs from being submitted to the hardware. > This must only affect jobs submitted by the killed process, not those > submitted by other processes. Yeah, that's exactly the plan here. For additional security we could safe the pid of the job submitter, but since this should basically not happen in normal operation I would rather like to avoid that. Christian.
On 05/18/2018 10:50 AM, Christian König wrote: > Am 18.05.2018 um 16:44 schrieb Michel Dänzer: >> On 2018-05-18 11:42 AM, Christian König wrote: >>>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the >>>> flush >>>> callback being called from multiple processes is an issue, maybe the >>>> flush callback isn't appropriate after all. >>> Userspace could also grab a reference just by opening /proc/$pid/fd/*. >>> >>> The idea is just that when any process which used the fd is killed by a >>> signal we drop the remaining jobs from being submitted to the hardware. >> This must only affect jobs submitted by the killed process, not those >> submitted by other processes. > > Yeah, that's exactly the plan here. I don't see how it's gong to happen - .flush is being called for any terminating process regardless if he submitted jobs or just accidentally (or not) has the device file FD in his private file table. So here we going to have a problem with that requirement. If a process is being killed and .flush is executed I don't have any way to know which amdgpu_ctx to chose to terminate it's pending jobs. The only info i have from .flush caller is the process id. As it's now in amdgpu_ctx_mgr_entity_fini and amdgpu_ctx_mgr_entity_cleanup we are going to iterate all the contextes from the context manager list and terminate them all, which sounds wrong to me indeed. I can save the pid of the context creator on the context structure so i can match during .flush call, but in case some one creates the context but passes the context id to another process for actual job submission this approach won't work either. Am I messing something here ? Andrey > > For additional security we could safe the pid of the job submitter, > but since this should basically not happen in normal operation I would > rather like to avoid that. > > Christian.
Am 18.05.2018 um 17:02 schrieb Andrey Grodzovsky: > > > On 05/18/2018 10:50 AM, Christian König wrote: >> Am 18.05.2018 um 16:44 schrieb Michel Dänzer: >>> On 2018-05-18 11:42 AM, Christian König wrote: >>>>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the >>>>> flush >>>>> callback being called from multiple processes is an issue, maybe the >>>>> flush callback isn't appropriate after all. >>>> Userspace could also grab a reference just by opening /proc/$pid/fd/*. >>>> >>>> The idea is just that when any process which used the fd is killed >>>> by a >>>> signal we drop the remaining jobs from being submitted to the >>>> hardware. >>> This must only affect jobs submitted by the killed process, not those >>> submitted by other processes. >> >> Yeah, that's exactly the plan here. > > I don't see how it's gong to happen - > .flush is being called for any terminating process regardless if he > submitted jobs > or just accidentally (or not) has the device file FD in his private > file table. So here > we going to have a problem with that requirement. If a process is > being killed and .flush is > executed I don't have any way to know which amdgpu_ctx to chose to > terminate it's pending jobs. > The only info i have from .flush caller is the process id. > As it's now in amdgpu_ctx_mgr_entity_fini and > amdgpu_ctx_mgr_entity_cleanup we are going to iterate > all the contextes from the context manager list and terminate them > all, which sounds wrong to me indeed. > I can save the pid of the context creator on the context structure so > i can match during .flush call, but in case some one > creates the context but passes the context id to another process for > actual job submission this approach won't work either. > > Am I messing something here ? Your analyses is correct, it's just that I think that this case should not happen. What can happen is that the fd is passed accidentally to child processes and those child processes are then killed, but passing the fd to child processes is a bug in the first place. When somebody on purpose opens the fd and kills the process then it breaks and he can keep the pieces. I mean to open the fd you need to be privileged anyway. What we could do to completely fix the issue: 1. Note for each submitted job which process (pid) it submitted. 2. During flush wait or kill only jobs of the current process. But I think that this is overkill. Christian. > > Andrey > >> >> For additional security we could safe the pid of the job submitter, >> but since this should basically not happen in normal operation I >> would rather like to avoid that. >> >> Christian. >
diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 5d8906d63..306541f33 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -200,12 +200,12 @@ open_hw(const char *dev) int fd; if (dev) - fd = open(dev, O_RDWR, 0); + fd = open(dev, O_RDWR | O_CLOEXEC, 0); else { dev = getenv("KMSDEVICE"); - if ((NULL == dev) || ((fd = open(dev, O_RDWR, 0)) == -1)) { + if ((NULL == dev) || ((fd = open(dev, O_RDWR | O_CLOEXEC, 0)) == -1)) { dev = "/dev/dri/card0"; - fd = open(dev, O_RDWR, 0); + fd = open(dev, O_RDWR | O_CLOEXEC, 0); } } if (fd == -1) diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c index 11af52c46..70374ace8 100644 --- a/hw/xfree86/os-support/linux/lnx_platform.c +++ b/hw/xfree86/os-support/linux/lnx_platform.c @@ -43,7 +43,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path, int delayed_index) } if (fd == -1) - fd = open(path, O_RDWR, O_CLOEXEC); + fd = open(path, O_RDWR | O_CLOEXEC, 0); if (fd == -1) return FALSE;