From patchwork Thu Nov 12 03:00:14 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andy Zhou X-Patchwork-Id: 543197 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id C0D3C1413F0 for ; Thu, 12 Nov 2015 14:00:20 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=nicira_com.20150623.gappssmtp.com header.i=@nicira_com.20150623.gappssmtp.com header.b=byJhb0NP; dkim-atps=neutral Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 6213310994; Wed, 11 Nov 2015 19:00:19 -0800 (PST) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 2958D106C9 for ; Wed, 11 Nov 2015 19:00:18 -0800 (PST) Received: from bar5.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id 38E541E01B0 for ; Wed, 11 Nov 2015 20:00:17 -0700 (MST) X-ASG-Debug-ID: 1447297216-09eadd03662ef4f0001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar5.cudamail.com with ESMTP id 5zrd3hPnTcAA7Y4o (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 11 Nov 2015 20:00:16 -0700 (MST) X-Barracuda-Envelope-From: azhou@nicira.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO mail-qk0-f180.google.com) (209.85.220.180) by mx1-pf1.cudamail.com with ESMTPS (RC4-SHA encrypted); 12 Nov 2015 03:00:16 -0000 Received-SPF: unknown (mx1-pf1.cudamail.com: Multiple SPF records returned) X-Barracuda-Apparent-Source-IP: 209.85.220.180 X-Barracuda-RBL-IP: 209.85.220.180 Received: by qkcl124 with SMTP id l124so19231187qkc.3 for ; Wed, 11 Nov 2015 19:00:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nicira_com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=RgnG0bKXcKboqoGOkeV/3kFayBcqUni/844xEcl2wfU=; b=byJhb0NPcj0fd2+0amjdRHE6A9UmimdW3QlMkCSqrCvMkbVSa5rlK+hEJM46Gqylhx ezNCU/U7u/U5MiNwuxJelmFy7IlFs3MWNe3Zz5Nq69EV4TSNTr9bSyUxzSYzWUHJVTQb 1PYjQHJJa0QFRok5DhttMvzyfiIZDfXJoydvG1AYL5o4OVMolX6aaQUKa4zozOiqwAJf CbbDCPvU0hhwtPVb05/kV1qtl17uwwpAT9Wn9BhkIJdMpJkh4KIr3gk8LfzgAo2lAB+u kUkx0k5b4c5Ln5EUFvZscp0+r/TFSsqi/q4mkN4JO7a0MXyLsBOqoRKVRASSqJisMMam 3ofg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=RgnG0bKXcKboqoGOkeV/3kFayBcqUni/844xEcl2wfU=; b=PVndyJ5uAJtdlP9gn2R2ltW+AhSOMJFVFvlTSA5bXZpIY2uzqPiYugkY9wfiOj0z8+ kVf8XzqxhxsT0UkQrC5sLhothLnG4Iuv2nRIIQZbdv9kEKJ2BqaTtgvsWYwypz+TjpMw c35aY2zEmigAcIFGDMT89Vqcv9yhzzOYfiCddRhSZwDq46dR8uFyOPVc0Qu74aoDMByd Csz3i6fVRFd2z3ax7gcrUPr/jNIdiJTbhVNm5MvjHABSP6xkkN1REgVDPpJ9zsiOjp1S 7oui6sWo6jtjty1FDS35cQmujYSeWngIzgDI+A1ovqyMEI+ySjG2bAjFM7uBY5iSVvPE uTQA== X-Gm-Message-State: ALoCoQnWccoU7sg2TGY0Y90VGvMJAe54q7guV2z9H9mgQATK/dDfdVQUMfsJOxta10vYnLWXQ+mM MIME-Version: 1.0 X-Received: by 10.55.33.210 with SMTP id f79mr11586592qki.25.1447297214980; Wed, 11 Nov 2015 19:00:14 -0800 (PST) Received: by 10.140.100.206 with HTTP; Wed, 11 Nov 2015 19:00:14 -0800 (PST) In-Reply-To: References: <1447280029-2598-1-git-send-email-azhou@nicira.com> <1447280029-2598-2-git-send-email-azhou@nicira.com> Date: Wed, 11 Nov 2015 19:00:14 -0800 Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-1110097924 X-CudaMail-DTE: 111115 X-CudaMail-Originating-IP: 209.85.220.180 X-CudaMail-Envelope-Sender: azhou@nicira.com X-ASG-Orig-Subj: [##CM-E1-1110097924##]Re: [ovs-dev] [additional --user changes v4 2/3] vlog: change log file owner when switching user From: Andy Zhou To: Gurucharan Shetty X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1447297216 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: "dev@openvswitch.org" Subject: Re: [ovs-dev] [additional --user changes v4 2/3] vlog: change log file owner when switching user X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On Wed, Nov 11, 2015 at 6:29 PM, Gurucharan Shetty wrote: > 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 wrote: >> On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka wrote: >>> >>> >>> On 11 November 2015 at 14:13, Andy Zhou 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 >>>> >>>> --- >>>> 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 >>> >>> Thanks for working on this, Andy. >> >> Thanks for your review. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev I see why. uid_t and gid_t are probably not defined on Windows. Since it is breaking the build, I pushed the following patch that will not compile vlog_change_owner() on windows. Sorry for breaking the build. diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h index bc16590..5309602 100644 --- a/include/openvswitch/vlog.h +++ b/include/openvswitch/vlog.h @@ -143,7 +143,9 @@ 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); +#ifndef _WIN32 +void vlog_change_owner_unix(uid_t, gid_t); +#endif /* 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 e69517a..a471b59 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -849,7 +849,7 @@ 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); + vlog_change_owner_unix(uid, gid); if (LINUX) { if (LIBCAPNG) { diff --git a/lib/vlog.c b/lib/vlog.c index 841c7a4..18d0e33 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -430,12 +430,13 @@ vlog_reopen_log_file(void) } } +#ifndef _WIN32 /* 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) +vlog_change_owner_unix(uid_t user, gid_t group) { if (!log_file_name) { return; @@ -450,6 +451,7 @@ vlog_change_owner(uid_t user, gid_t group) log_file_name, ovs_strerror(errno)); } } +#endif /* Set debugging levels. Returns null if successful, otherwise an error * message that the caller must free(). */