| Message ID | 20200704044333.74479-1-hepeng.0320@bytedance.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack-tp: fix lock order in conn_update_expiration | expand |
Bleep bloop. Greetings hepeng.0320, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author hepeng.0320 <hepeng.0320@bytedance.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: hepeng.0320 <xnhp0320@gmail.com> Lines checked: 42, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Sat, Jul 04, 2020 at 12:43:33PM +0800, hepeng.0320 wrote: > *conn_update_expiration* violates the lock order of conn->lock and > ct->lock. In the comments of conntrack, the conn->lock should be > held after ct->lock when ct->lock needs to be taken. > > Signed-off-by: hepeng.0320 <xnhp0320@gmail.com> > --- Thanks, LGTM. Can you fix the Signed-off-by and resend? William > lib/conntrack-tp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c > index 3a7604c0d..a586d3a8d 100644 > --- a/lib/conntrack-tp.c > +++ b/lib/conntrack-tp.c > @@ -260,15 +260,20 @@ conn_update_expiration(struct conntrack *ct, struct conn *conn, > struct timeout_policy *tp; > uint32_t val; > > + ovs_mutex_unlock(&conn->lock); > + > ovs_mutex_lock(&ct->ct_lock); > + ovs_mutex_lock(&conn->lock); > tp = timeout_policy_lookup(ct, conn->tp_id); > if (tp) { > val = tp->policy.attrs[tm_to_ct_dpif_tp(tm)]; > } else { > val = ct_dpif_netdev_tp_def[tm_to_ct_dpif_tp(tm)]; > } > + ovs_mutex_unlock(&conn->lock); > ovs_mutex_unlock(&ct->ct_lock); > > + ovs_mutex_lock(&conn->lock); > VLOG_DBG_RL(&rl, "Update timeout %s zone=%u with policy id=%d " > "val=%u sec.", > ct_timeout_str[tm], conn->key.zone, conn->tp_id, val); > -- > 2.20.1 >
diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c index 3a7604c0d..a586d3a8d 100644 --- a/lib/conntrack-tp.c +++ b/lib/conntrack-tp.c @@ -260,15 +260,20 @@ conn_update_expiration(struct conntrack *ct, struct conn *conn, struct timeout_policy *tp; uint32_t val; + ovs_mutex_unlock(&conn->lock); + ovs_mutex_lock(&ct->ct_lock); + ovs_mutex_lock(&conn->lock); tp = timeout_policy_lookup(ct, conn->tp_id); if (tp) { val = tp->policy.attrs[tm_to_ct_dpif_tp(tm)]; } else { val = ct_dpif_netdev_tp_def[tm_to_ct_dpif_tp(tm)]; } + ovs_mutex_unlock(&conn->lock); ovs_mutex_unlock(&ct->ct_lock); + ovs_mutex_lock(&conn->lock); VLOG_DBG_RL(&rl, "Update timeout %s zone=%u with policy id=%d " "val=%u sec.", ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
*conn_update_expiration* violates the lock order of conn->lock and ct->lock. In the comments of conntrack, the conn->lock should be held after ct->lock when ct->lock needs to be taken. Signed-off-by: hepeng.0320 <xnhp0320@gmail.com> --- lib/conntrack-tp.c | 5 +++++ 1 file changed, 5 insertions(+)