| Message ID | 1447649984-6136-1-git-send-email-azhou@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
Thanks for the fix, Andy Acked-by: Daniele Di Proietto <diproiettod@vmware.com> On 15/11/2015 20:59, "Andy Zhou" <azhou@nicira.com> wrote: >Calling VLOG_FATAL() while holding the 'log_file_mutex" may lead to >deadlock since VLOG_FATAL() implementation tries to acquire the >same lock. Fix this by building the error message first, then >call VLOG_FATAL() after the 'log_file_mutex' has been released. > >This bug is not likely show up in practice since chown() usually >won't fail. It is still better to have a correct implementation. > >Reported-by: Daniele Di Proietto <ddiproietto@vmware.com> >Signed-off-by: Andy Zhou <azhou@nicira.com> >--- > lib/vlog.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > >diff --git a/lib/vlog.c b/lib/vlog.c >index a4aa2a0..28cea5d 100644 >--- a/lib/vlog.c >+++ b/lib/vlog.c >@@ -438,15 +438,24 @@ vlog_reopen_log_file(void) > void > vlog_change_owner_unix(uid_t user, gid_t group) > { >- ovs_mutex_lock(&log_file_mutex); >- int error = log_file_name ? chown(log_file_name, user, group) : 0; >+ struct ds err = DS_EMPTY_INITIALIZER; >+ int error; > >+ ovs_mutex_lock(&log_file_mutex); >+ error = log_file_name ? chown(log_file_name, user, group) : 0; > if (error) { >- VLOG_FATAL("Failed to change %s ownership: %s.", >- log_file_name, ovs_strerror(errno)); >+ /* Build the error message. We can not call VLOG_FATAL directly >+ * here because VLOG_FATAL() will try again to to acquire >+ * 'log_file_mutex' lock, causing deadlock. >+ */ >+ ds_put_format(&err, "Failed to change %s ownership: %s.", >+ log_file_name, ovs_strerror(errno)); > } >- > ovs_mutex_unlock(&log_file_mutex); >+ >+ if (error) { >+ VLOG_FATAL("%s", ds_steal_cstr(&err)); >+ } > } > #endif > >-- >1.9.1 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Sm >B5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=z501EObTNBEFvIFC6CiugNNwuDj1_Q >EkKFHBzmREoC4&s=XawONBR44io3PX3YhsJonjY8eTVa5qWu3O2Ud3eEVyc&e=
On Thu, Nov 19, 2015 at 11:30 AM, Daniele Di Proietto < diproiettod@vmware.com> wrote: > Thanks for the fix, Andy > > Acked-by: Daniele Di Proietto <diproiettod@vmware.com> > > > Thanks. pushed to master.
diff --git a/lib/vlog.c b/lib/vlog.c index a4aa2a0..28cea5d 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -438,15 +438,24 @@ vlog_reopen_log_file(void) void vlog_change_owner_unix(uid_t user, gid_t group) { - ovs_mutex_lock(&log_file_mutex); - int error = log_file_name ? chown(log_file_name, user, group) : 0; + struct ds err = DS_EMPTY_INITIALIZER; + int error; + ovs_mutex_lock(&log_file_mutex); + error = log_file_name ? chown(log_file_name, user, group) : 0; if (error) { - VLOG_FATAL("Failed to change %s ownership: %s.", - log_file_name, ovs_strerror(errno)); + /* Build the error message. We can not call VLOG_FATAL directly + * here because VLOG_FATAL() will try again to to acquire + * 'log_file_mutex' lock, causing deadlock. + */ + ds_put_format(&err, "Failed to change %s ownership: %s.", + log_file_name, ovs_strerror(errno)); } - ovs_mutex_unlock(&log_file_mutex); + + if (error) { + VLOG_FATAL("%s", ds_steal_cstr(&err)); + } } #endif
Calling VLOG_FATAL() while holding the 'log_file_mutex" may lead to deadlock since VLOG_FATAL() implementation tries to acquire the same lock. Fix this by building the error message first, then call VLOG_FATAL() after the 'log_file_mutex' has been released. This bug is not likely show up in practice since chown() usually won't fail. It is still better to have a correct implementation. Reported-by: Daniele Di Proietto <ddiproietto@vmware.com> Signed-off-by: Andy Zhou <azhou@nicira.com> --- lib/vlog.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)