* [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC @ 2024-05-07 11:08 Andrew Cooper 2024-05-07 11:32 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 6+ messages in thread From: Andrew Cooper @ 2024-05-07 11:08 UTC (permalink / raw) To: Xen-devel Cc: Andrew Cooper, Demi Marie Obenour, Anthony PERARD, Juergen Gross, Marek Marczykowski-Górecki `xl devd` has been observed leaking /var/log/xldevd.log into children. Link: https://github.com/QubesOS/qubes-issues/issues/8292 Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Anthony PERARD <anthony@xenproject.org> CC: Juergen Gross <jgross@suse.com> CC: Demi Marie Obenour <demi@invisiblethingslab.com> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Also entirely speculative based on the QubesOS ticket. --- tools/xl/xl_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c index 17489d182954..060186db3a59 100644 --- a/tools/xl/xl_utils.c +++ b/tools/xl/xl_utils.c @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) exit(-1); } - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); free(fullname); assert(logfile >= 3); base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81 prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919 -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC 2024-05-07 11:08 [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC Andrew Cooper @ 2024-05-07 11:32 ` Marek Marczykowski-Górecki 2024-05-07 14:15 ` Andrew Cooper 2024-05-07 14:26 ` Marek Marczykowski-Górecki 0 siblings, 2 replies; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2024-05-07 11:32 UTC (permalink / raw) To: Andrew Cooper Cc: Xen-devel, Demi Marie Obenour, Anthony PERARD, Juergen Gross [-- Attachment #1: Type: text/plain, Size: 2855 bytes --] On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: > `xl devd` has been observed leaking /var/log/xldevd.log into children. > > Link: https://github.com/QubesOS/qubes-issues/issues/8292 > Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Anthony PERARD <anthony@xenproject.org> > CC: Juergen Gross <jgross@suse.com> > CC: Demi Marie Obenour <demi@invisiblethingslab.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > Also entirely speculative based on the QubesOS ticket. > --- > tools/xl/xl_utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c > index 17489d182954..060186db3a59 100644 > --- a/tools/xl/xl_utils.c > +++ b/tools/xl/xl_utils.c > @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) > exit(-1); > } > > - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); > + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); This one might be not enough, as the FD gets dup2()-ed to stdout/stderr just outside of the context here, and then inherited by various hotplug script. Just adding O_CLOEXEC here means the hotplug scripts will run with stdout/stderr closed. The scripts shipped with Xen do redirect stderr to a log quite early, but a) it doesn't do it for stdout, and b) custom hotplug scripts are a valid use case. Without that, I see at least few potential issues: - some log messages may be lost (minor, but annoying) - something might simply fail on writing to a closed FD, breaking the hotplug script - FD 1 will be used as first free FD for any open() or similar call - if a tool later tries writing something to stdout, it will gets written to that FD - worse of all three What should be the behavior of hotplug scripts logging? Should they always take care of their own logging? If so, the hotplug calling part should redirect stdout/stderr to /dev/null IMO. But if `xl` should provide some default logging for them (like, the xldevd.log here?), then the O_CLOEXEC should be set only after duplicating logfile over stdout/err. > free(fullname); > assert(logfile >= 3); > > > base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81 > prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919 Which one is this? I don't see it in staging, nor in any of your branches on xenbits. Lore finds "tools/libxs: Open /dev/xen/xenbus fds as O_CLOEXEC" which I guess is correct, but I have no idea how it correlates it, as this hash doesn't appear anywhere in the message, nor its headers... -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC 2024-05-07 11:32 ` Marek Marczykowski-Górecki @ 2024-05-07 14:15 ` Andrew Cooper 2024-05-07 14:23 ` Marek Marczykowski-Górecki 2024-05-07 14:26 ` Marek Marczykowski-Górecki 1 sibling, 1 reply; 6+ messages in thread From: Andrew Cooper @ 2024-05-07 14:15 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Xen-devel, Demi Marie Obenour, Anthony PERARD, Juergen Gross On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote: > On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: >> `xl devd` has been observed leaking /var/log/xldevd.log into children. >> >> Link: https://github.com/QubesOS/qubes-issues/issues/8292 >> Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Anthony PERARD <anthony@xenproject.org> >> CC: Juergen Gross <jgross@suse.com> >> CC: Demi Marie Obenour <demi@invisiblethingslab.com> >> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> >> Also entirely speculative based on the QubesOS ticket. >> --- >> tools/xl/xl_utils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c >> index 17489d182954..060186db3a59 100644 >> --- a/tools/xl/xl_utils.c >> +++ b/tools/xl/xl_utils.c >> @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) >> exit(-1); >> } >> >> - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); >> + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); > This one might be not enough, as the FD gets dup2()-ed to stdout/stderr > just outside of the context here, and then inherited by various hotplug > script. Just adding O_CLOEXEC here means the hotplug scripts will run > with stdout/stderr closed. Lovely :( Yes - this won't work. I guess what we want instead is: diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c index 060186db3a59..a0ce7dd7fa21 100644 --- a/tools/xl/xl_utils.c +++ b/tools/xl/xl_utils.c @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile) dup2(logfile, 2); close(nullfd); + close(logfile); CHK_SYSCALL(daemon(0, 1)); which at least means there's not a random extra fd attached to the logfile. >> free(fullname); >> assert(logfile >= 3); >> >> >> base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81 >> prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919 > Which one is this? I don't see it in staging, nor in any of your > branches on xenbits. Lore finds "tools/libxs: Open /dev/xen/xenbus fds > as O_CLOEXEC" which I guess is correct, but I have no idea how it > correlates it, as this hash doesn't appear anywhere in the message, nor > its headers... It's the libxs patch, but rebased over yesterday's push to staging. auto-base doesn't work quite so well in cases like this. ~Andrew ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC 2024-05-07 14:15 ` Andrew Cooper @ 2024-05-07 14:23 ` Marek Marczykowski-Górecki 2024-05-07 14:26 ` Andrew Cooper 0 siblings, 1 reply; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2024-05-07 14:23 UTC (permalink / raw) To: Andrew Cooper Cc: Xen-devel, Demi Marie Obenour, Anthony PERARD, Juergen Gross [-- Attachment #1: Type: text/plain, Size: 2304 bytes --] On Tue, May 07, 2024 at 03:15:48PM +0100, Andrew Cooper wrote: > On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote: > > On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: > >> `xl devd` has been observed leaking /var/log/xldevd.log into children. > >> > >> Link: https://github.com/QubesOS/qubes-issues/issues/8292 > >> Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Anthony PERARD <anthony@xenproject.org> > >> CC: Juergen Gross <jgross@suse.com> > >> CC: Demi Marie Obenour <demi@invisiblethingslab.com> > >> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > >> > >> Also entirely speculative based on the QubesOS ticket. > >> --- > >> tools/xl/xl_utils.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c > >> index 17489d182954..060186db3a59 100644 > >> --- a/tools/xl/xl_utils.c > >> +++ b/tools/xl/xl_utils.c > >> @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) > >> exit(-1); > >> } > >> > >> - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); > >> + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); > > This one might be not enough, as the FD gets dup2()-ed to stdout/stderr > > just outside of the context here, and then inherited by various hotplug > > script. Just adding O_CLOEXEC here means the hotplug scripts will run > > with stdout/stderr closed. > > Lovely :( Yes - this won't work. I guess what we want instead is: > > diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c > index 060186db3a59..a0ce7dd7fa21 100644 > --- a/tools/xl/xl_utils.c > +++ b/tools/xl/xl_utils.c > @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile) > dup2(logfile, 2); > > close(nullfd); > + close(logfile); > > CHK_SYSCALL(daemon(0, 1)); > > which at least means there's not a random extra fd attached to the logfile. But logfile is a global variable, and it looks to be used in dolog()... -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC 2024-05-07 14:23 ` Marek Marczykowski-Górecki @ 2024-05-07 14:26 ` Andrew Cooper 0 siblings, 0 replies; 6+ messages in thread From: Andrew Cooper @ 2024-05-07 14:26 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Xen-devel, Demi Marie Obenour, Anthony PERARD, Juergen Gross On 07/05/2024 3:23 pm, Marek Marczykowski-Górecki wrote: > On Tue, May 07, 2024 at 03:15:48PM +0100, Andrew Cooper wrote: >> On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote: >>> On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: >>>> `xl devd` has been observed leaking /var/log/xldevd.log into children. >>>> >>>> Link: https://github.com/QubesOS/qubes-issues/issues/8292 >>>> Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Anthony PERARD <anthony@xenproject.org> >>>> CC: Juergen Gross <jgross@suse.com> >>>> CC: Demi Marie Obenour <demi@invisiblethingslab.com> >>>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>>> >>>> Also entirely speculative based on the QubesOS ticket. >>>> --- >>>> tools/xl/xl_utils.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c >>>> index 17489d182954..060186db3a59 100644 >>>> --- a/tools/xl/xl_utils.c >>>> +++ b/tools/xl/xl_utils.c >>>> @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) >>>> exit(-1); >>>> } >>>> >>>> - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); >>>> + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); >>> This one might be not enough, as the FD gets dup2()-ed to stdout/stderr >>> just outside of the context here, and then inherited by various hotplug >>> script. Just adding O_CLOEXEC here means the hotplug scripts will run >>> with stdout/stderr closed. >> Lovely :( Yes - this won't work. I guess what we want instead is: >> >> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c >> index 060186db3a59..a0ce7dd7fa21 100644 >> --- a/tools/xl/xl_utils.c >> +++ b/tools/xl/xl_utils.c >> @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile) >> dup2(logfile, 2); >> >> close(nullfd); >> + close(logfile); >> >> CHK_SYSCALL(daemon(0, 1)); >> >> which at least means there's not a random extra fd attached to the logfile. > But logfile is a global variable, and it looks to be used in dolog()... Urgh, fine. Lets go back to your suggestion of setting CLOEXEC after dup()ing. ~Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC 2024-05-07 11:32 ` Marek Marczykowski-Górecki 2024-05-07 14:15 ` Andrew Cooper @ 2024-05-07 14:26 ` Marek Marczykowski-Górecki 1 sibling, 0 replies; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2024-05-07 14:26 UTC (permalink / raw) To: Andrew Cooper Cc: Xen-devel, Demi Marie Obenour, Anthony PERARD, Juergen Gross [-- Attachment #1: Type: text/plain, Size: 3268 bytes --] On Tue, May 07, 2024 at 01:32:00PM +0200, Marek Marczykowski-Górecki wrote: > On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: > > `xl devd` has been observed leaking /var/log/xldevd.log into children. > > > > Link: https://github.com/QubesOS/qubes-issues/issues/8292 > > Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > CC: Anthony PERARD <anthony@xenproject.org> > > CC: Juergen Gross <jgross@suse.com> > > CC: Demi Marie Obenour <demi@invisiblethingslab.com> > > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > Also entirely speculative based on the QubesOS ticket. > > --- > > tools/xl/xl_utils.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c > > index 17489d182954..060186db3a59 100644 > > --- a/tools/xl/xl_utils.c > > +++ b/tools/xl/xl_utils.c > > @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) > > exit(-1); > > } > > > > - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); > > + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC, 0644)); > > This one might be not enough, as the FD gets dup2()-ed to stdout/stderr > just outside of the context here, and then inherited by various hotplug > script. Just adding O_CLOEXEC here means the hotplug scripts will run > with stdout/stderr closed. The scripts shipped with Xen do redirect > stderr to a log quite early, but a) it doesn't do it for stdout, and b) > custom hotplug scripts are a valid use case. > Without that, I see at least few potential issues: > - some log messages may be lost (minor, but annoying) > - something might simply fail on writing to a closed FD, breaking the > hotplug script > - FD 1 will be used as first free FD for any open() or similar call - if > a tool later tries writing something to stdout, it will gets written > to that FD - worse of all three Wait, the above is wrong, dup does not copy the O_CLOEXEC flag over to the new FD. So, maybe your patch is correct after all. > What should be the behavior of hotplug scripts logging? Should they > always take care of their own logging? If so, the hotplug calling part > should redirect stdout/stderr to /dev/null IMO. But if `xl` should > provide some default logging for them (like, the xldevd.log here?), then > the O_CLOEXEC should be set only after duplicating logfile over stdout/err. > > > free(fullname); > > assert(logfile >= 3); > > > > > > base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81 > > prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919 > > Which one is this? I don't see it in staging, nor in any of your > branches on xenbits. Lore finds "tools/libxs: Open /dev/xen/xenbus fds > as O_CLOEXEC" which I guess is correct, but I have no idea how it > correlates it, as this hash doesn't appear anywhere in the message, nor > its headers... > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-07 14:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-07 11:08 [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC Andrew Cooper 2024-05-07 11:32 ` Marek Marczykowski-Górecki 2024-05-07 14:15 ` Andrew Cooper 2024-05-07 14:23 ` Marek Marczykowski-Górecki 2024-05-07 14:26 ` Andrew Cooper 2024-05-07 14:26 ` Marek Marczykowski-Górecki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.