| Message ID | 1447280029-2598-2-git-send-email-azhou@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
On 11 November 2015 at 14:13, Andy Zhou <azhou@nicira.com> wrote: > vlog log file can be created when parsing --log-file option, before > switching user, in case the --user option is also specified. While this > does not directly cause errors for the running daemons, it can > leave the log files on the disk as created under the "root" user. > This patch fix the log file ownership to the user specified with --user. > > Signed-off-by: Andy Zhou <azhou@nicira.com> > > --- > v1->v2: Add a comment on vlog_change_owner return code. > v2->v3: no change > v3->v4: Reword the commit message. change vlog_change_owner() to void. > --- > include/openvswitch/vlog.h | 1 + > lib/daemon-unix.c | 6 +++++- > lib/vlog.c | 22 +++++++++++++++++++++- > 3 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h > index f6bb3ab..bc16590 100644 > --- a/include/openvswitch/vlog.h > +++ b/include/openvswitch/vlog.h > @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); > void vlog_set_pattern(enum vlog_destination, const char *pattern); > int vlog_set_log_file(const char *file_name); > int vlog_reopen_log_file(void); > +void vlog_change_owner(uid_t, gid_t); > > /* Configure method how vlog should send messages to syslog server. */ > void vlog_set_syslog_method(const char *method); > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index 0125745..e69517a 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -739,7 +739,7 @@ daemon_switch_group(gid_t real, gid_t effective, > { > if ((setresgid(real, effective, saved) == -1) || > !gid_verify(real, effective, saved)) { > - VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", > + VLOG_FATAL("%s: failed to switch group to gid as %d, aborting", > pidfile, gid); > } > } > @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath > OVS_UNUSED) > static void > daemon_become_new_user__(bool access_datapath) > { > + /* If vlog file has been created, change its owner to the non-root > user > + * as specifed by the --user option. */ > + vlog_change_owner(uid, gid); > + > if (LINUX) { > if (LIBCAPNG) { > daemon_become_new_user_linux(access_datapath); > diff --git a/lib/vlog.c b/lib/vlog.c > index da31e6f..ade0a45 100644 > --- a/lib/vlog.c > +++ b/lib/vlog.c > @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, > 0); > * All of the following is protected by 'log_file_mutex', which nests > inside > * pattern_rwlock. */ > static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER; > -static char *log_file_name OVS_GUARDED_BY(log_file_mutex); > +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex); > static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1; > static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex); > static bool log_async OVS_GUARDED_BY(log_file_mutex); > @@ -430,6 +430,26 @@ vlog_reopen_log_file(void) > } > } > > +/* In case a log file exists, change its owner to new 'user' and 'group'. > + * > + * This is useful for handling cases where the --log-file option is > + * specified ahead of the --user option. */ > +void > +vlog_change_owner(uid_t user, gid_t group) > +{ > + int error = 0; > + > + if (log_file_name) { > + ovs_mutex_lock(&log_file_mutex); > + error = chown(log_file_name, user, group); > + ovs_mutex_unlock(&log_file_mutex); > + } > + > + if (error) { > + VLOG_FATAL("Failed to change log file ownership."); > I would print errno value here and the file name you are actually trying to change the ownership for. It would simply provide a hint to the users on what was actually wrong, if it failed. VLOG_FATAL("Failed to change %s ownership: %s", log_file_name, ovs_strerror(errno)); And early return from function if log_file_name is NULL to make code look better. + } > +} > + > Otherwise, Acked-by: Ansis Atteka <aatteka@nicira.com> Thanks for working on this, Andy.
On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <ansisatteka@gmail.com> wrote: > > > On 11 November 2015 at 14:13, Andy Zhou <azhou@nicira.com> wrote: >> >> vlog log file can be created when parsing --log-file option, before >> switching user, in case the --user option is also specified. While this >> does not directly cause errors for the running daemons, it can >> leave the log files on the disk as created under the "root" user. >> This patch fix the log file ownership to the user specified with --user. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> >> >> --- >> v1->v2: Add a comment on vlog_change_owner return code. >> v2->v3: no change >> v3->v4: Reword the commit message. change vlog_change_owner() to void. >> --- >> include/openvswitch/vlog.h | 1 + >> lib/daemon-unix.c | 6 +++++- >> lib/vlog.c | 22 +++++++++++++++++++++- >> 3 files changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h >> index f6bb3ab..bc16590 100644 >> --- a/include/openvswitch/vlog.h >> +++ b/include/openvswitch/vlog.h >> @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); >> void vlog_set_pattern(enum vlog_destination, const char *pattern); >> int vlog_set_log_file(const char *file_name); >> int vlog_reopen_log_file(void); >> +void vlog_change_owner(uid_t, gid_t); >> >> /* Configure method how vlog should send messages to syslog server. */ >> void vlog_set_syslog_method(const char *method); >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >> index 0125745..e69517a 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -739,7 +739,7 @@ daemon_switch_group(gid_t real, gid_t effective, >> { >> if ((setresgid(real, effective, saved) == -1) || >> !gid_verify(real, effective, saved)) { >> - VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", >> + VLOG_FATAL("%s: failed to switch group to gid as %d, aborting", >> pidfile, gid); >> } >> } >> @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath >> OVS_UNUSED) >> static void >> daemon_become_new_user__(bool access_datapath) >> { >> + /* If vlog file has been created, change its owner to the non-root >> user >> + * as specifed by the --user option. */ >> + vlog_change_owner(uid, gid); >> + >> if (LINUX) { >> if (LIBCAPNG) { >> daemon_become_new_user_linux(access_datapath); >> diff --git a/lib/vlog.c b/lib/vlog.c >> index da31e6f..ade0a45 100644 >> --- a/lib/vlog.c >> +++ b/lib/vlog.c >> @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, >> 0); >> * All of the following is protected by 'log_file_mutex', which nests >> inside >> * pattern_rwlock. */ >> static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER; >> -static char *log_file_name OVS_GUARDED_BY(log_file_mutex); >> +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex); >> static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1; >> static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex); >> static bool log_async OVS_GUARDED_BY(log_file_mutex); >> @@ -430,6 +430,26 @@ vlog_reopen_log_file(void) >> } >> } >> >> +/* In case a log file exists, change its owner to new 'user' and 'group'. >> + * >> + * This is useful for handling cases where the --log-file option is >> + * specified ahead of the --user option. */ >> +void >> +vlog_change_owner(uid_t user, gid_t group) >> +{ >> + int error = 0; >> + >> + if (log_file_name) { >> + ovs_mutex_lock(&log_file_mutex); >> + error = chown(log_file_name, user, group); >> + ovs_mutex_unlock(&log_file_mutex); >> + } >> + >> + if (error) { >> + VLOG_FATAL("Failed to change log file ownership."); > > I would print errno value here and the file name you are actually trying to > change the ownership for. It would simply provide a hint to the users on > what was actually wrong, if it failed. > > VLOG_FATAL("Failed to change %s ownership: %s", log_file_name, > ovs_strerror(errno)); > > And early return from function if log_file_name is NULL to make code look > better. > Pushed to master with both changes as suggested. >> + } >> +} >> + > > Otherwise, Acked-by: Ansis Atteka <aatteka@nicira.com> > > Thanks for working on this, Andy. Thanks for your review.
One of the patches seems to break windows build. https://ci.appveyor.com/project/blp/ovs/build/1.0.916 On Wed, Nov 11, 2015 at 6:12 PM, Andy Zhou <azhou@nicira.com> wrote: > On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <ansisatteka@gmail.com> wrote: >> >> >> On 11 November 2015 at 14:13, Andy Zhou <azhou@nicira.com> wrote: >>> >>> vlog log file can be created when parsing --log-file option, before >>> switching user, in case the --user option is also specified. While this >>> does not directly cause errors for the running daemons, it can >>> leave the log files on the disk as created under the "root" user. >>> This patch fix the log file ownership to the user specified with --user. >>> >>> Signed-off-by: Andy Zhou <azhou@nicira.com> >>> >>> --- >>> v1->v2: Add a comment on vlog_change_owner return code. >>> v2->v3: no change >>> v3->v4: Reword the commit message. change vlog_change_owner() to void. >>> --- >>> include/openvswitch/vlog.h | 1 + >>> lib/daemon-unix.c | 6 +++++- >>> lib/vlog.c | 22 +++++++++++++++++++++- >>> 3 files changed, 27 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h >>> index f6bb3ab..bc16590 100644 >>> --- a/include/openvswitch/vlog.h >>> +++ b/include/openvswitch/vlog.h >>> @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); >>> void vlog_set_pattern(enum vlog_destination, const char *pattern); >>> int vlog_set_log_file(const char *file_name); >>> int vlog_reopen_log_file(void); >>> +void vlog_change_owner(uid_t, gid_t); >>> >>> /* Configure method how vlog should send messages to syslog server. */ >>> void vlog_set_syslog_method(const char *method); >>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >>> index 0125745..e69517a 100644 >>> --- a/lib/daemon-unix.c >>> +++ b/lib/daemon-unix.c >>> @@ -739,7 +739,7 @@ daemon_switch_group(gid_t real, gid_t effective, >>> { >>> if ((setresgid(real, effective, saved) == -1) || >>> !gid_verify(real, effective, saved)) { >>> - VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", >>> + VLOG_FATAL("%s: failed to switch group to gid as %d, aborting", >>> pidfile, gid); >>> } >>> } >>> @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath >>> OVS_UNUSED) >>> static void >>> daemon_become_new_user__(bool access_datapath) >>> { >>> + /* If vlog file has been created, change its owner to the non-root >>> user >>> + * as specifed by the --user option. */ >>> + vlog_change_owner(uid, gid); >>> + >>> if (LINUX) { >>> if (LIBCAPNG) { >>> daemon_become_new_user_linux(access_datapath); >>> diff --git a/lib/vlog.c b/lib/vlog.c >>> index da31e6f..ade0a45 100644 >>> --- a/lib/vlog.c >>> +++ b/lib/vlog.c >>> @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, >>> 0); >>> * All of the following is protected by 'log_file_mutex', which nests >>> inside >>> * pattern_rwlock. */ >>> static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER; >>> -static char *log_file_name OVS_GUARDED_BY(log_file_mutex); >>> +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex); >>> static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1; >>> static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex); >>> static bool log_async OVS_GUARDED_BY(log_file_mutex); >>> @@ -430,6 +430,26 @@ vlog_reopen_log_file(void) >>> } >>> } >>> >>> +/* In case a log file exists, change its owner to new 'user' and 'group'. >>> + * >>> + * This is useful for handling cases where the --log-file option is >>> + * specified ahead of the --user option. */ >>> +void >>> +vlog_change_owner(uid_t user, gid_t group) >>> +{ >>> + int error = 0; >>> + >>> + if (log_file_name) { >>> + ovs_mutex_lock(&log_file_mutex); >>> + error = chown(log_file_name, user, group); >>> + ovs_mutex_unlock(&log_file_mutex); >>> + } >>> + >>> + if (error) { >>> + VLOG_FATAL("Failed to change log file ownership."); >> >> I would print errno value here and the file name you are actually trying to >> change the ownership for. It would simply provide a hint to the users on >> what was actually wrong, if it failed. >> >> VLOG_FATAL("Failed to change %s ownership: %s", log_file_name, >> ovs_strerror(errno)); >> >> And early return from function if log_file_name is NULL to make code look >> better. >> > > Pushed to master with both changes as suggested. >>> + } >>> +} >>> + >> >> Otherwise, Acked-by: Ansis Atteka <aatteka@nicira.com> >> >> Thanks for working on this, Andy. > > Thanks for your review. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h index f6bb3ab..bc16590 100644 --- a/include/openvswitch/vlog.h +++ b/include/openvswitch/vlog.h @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); void vlog_set_pattern(enum vlog_destination, const char *pattern); int vlog_set_log_file(const char *file_name); int vlog_reopen_log_file(void); +void vlog_change_owner(uid_t, gid_t); /* Configure method how vlog should send messages to syslog server. */ void vlog_set_syslog_method(const char *method); diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index 0125745..e69517a 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -739,7 +739,7 @@ daemon_switch_group(gid_t real, gid_t effective, { if ((setresgid(real, effective, saved) == -1) || !gid_verify(real, effective, saved)) { - VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", + VLOG_FATAL("%s: failed to switch group to gid as %d, aborting", pidfile, gid); } } @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath OVS_UNUSED) static void daemon_become_new_user__(bool access_datapath) { + /* If vlog file has been created, change its owner to the non-root user + * as specifed by the --user option. */ + vlog_change_owner(uid, gid); + if (LINUX) { if (LIBCAPNG) { daemon_become_new_user_linux(access_datapath); diff --git a/lib/vlog.c b/lib/vlog.c index da31e6f..ade0a45 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0); * All of the following is protected by 'log_file_mutex', which nests inside * pattern_rwlock. */ static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER; -static char *log_file_name OVS_GUARDED_BY(log_file_mutex); +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex); static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1; static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex); static bool log_async OVS_GUARDED_BY(log_file_mutex); @@ -430,6 +430,26 @@ vlog_reopen_log_file(void) } } +/* In case a log file exists, change its owner to new 'user' and 'group'. + * + * This is useful for handling cases where the --log-file option is + * specified ahead of the --user option. */ +void +vlog_change_owner(uid_t user, gid_t group) +{ + int error = 0; + + if (log_file_name) { + ovs_mutex_lock(&log_file_mutex); + error = chown(log_file_name, user, group); + ovs_mutex_unlock(&log_file_mutex); + } + + if (error) { + VLOG_FATAL("Failed to change log file ownership."); + } +} + /* Set debugging levels. Returns null if successful, otherwise an error * message that the caller must free(). */ char *
vlog log file can be created when parsing --log-file option, before switching user, in case the --user option is also specified. While this does not directly cause errors for the running daemons, it can leave the log files on the disk as created under the "root" user. This patch fix the log file ownership to the user specified with --user. Signed-off-by: Andy Zhou <azhou@nicira.com> --- v1->v2: Add a comment on vlog_change_owner return code. v2->v3: no change v3->v4: Reword the commit message. change vlog_change_owner() to void. --- include/openvswitch/vlog.h | 1 + lib/daemon-unix.c | 6 +++++- lib/vlog.c | 22 +++++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-)