| Message ID | 20171025100145.GA144703@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 25, 2017 at 12:01 PM, Kees Cook <keescook@chromium.org> wrote: > sess->time2retain_timer.expires = > (get_jiffies_64() + sess->sess_ops->DefaultTime2Retain * HZ); > add_timer(&sess->time2retain_timer); > cmd->dataout_timer.expires = (get_jiffies_64() + na->dataout_timeout * HZ); > add_timer(&cmd->dataout_timer); > np->np_login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); > add_timer(&np->np_login_timer); > + timeout.timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); > + add_timer(&timeout.timer); > conn->nopin_response_timer.expires = > (get_jiffies_64() + na->nopin_response_timeout * HZ); > add_timer(&conn->nopin_response_timer); > conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); > add_timer(&conn->nopin_timer); > conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); > add_timer(&conn->nopin_timer); I assume you're trying to keep these diffs as small as possible, but I imagine at some point the above can also be combined into a single mod_timer. Also, all the goofiness with the flags stuff seems like an antique replacement for timer_pending: > conn->nopin_response_timer_flags &= ~ISCSI_TF_STOP; > conn->nopin_response_timer_flags |= ISCSI_TF_RUNNING; -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 25, 2017 at 2:41 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > On Wed, Oct 25, 2017 at 12:01 PM, Kees Cook <keescook@chromium.org> wrote: >> sess->time2retain_timer.expires = >> (get_jiffies_64() + sess->sess_ops->DefaultTime2Retain * HZ); >> add_timer(&sess->time2retain_timer); >> cmd->dataout_timer.expires = (get_jiffies_64() + na->dataout_timeout * HZ); >> add_timer(&cmd->dataout_timer); >> np->np_login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); >> add_timer(&np->np_login_timer); >> + timeout.timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); >> + add_timer(&timeout.timer); >> conn->nopin_response_timer.expires = >> (get_jiffies_64() + na->nopin_response_timeout * HZ); >> add_timer(&conn->nopin_response_timer); >> conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); >> add_timer(&conn->nopin_timer); >> conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); >> add_timer(&conn->nopin_timer); > > I assume you're trying to keep these diffs as small as possible, but I > imagine at some point the above can also be combined into a single > mod_timer. Yeah, I've resisted making those changes for this set of conversions. > Also, all the goofiness with the flags stuff seems like an antique > replacement for timer_pending: > >> conn->nopin_response_timer_flags &= ~ISCSI_TF_STOP; >> conn->nopin_response_timer_flags |= ISCSI_TF_RUNNING; Those appear to be separate flags not from struct timer_list. However, maintainers: sorry to send this one -- it can't be merged yet, this uses timer_setup_on_stack() which is only in -next right now. If it looks okay, though, I can carry this in the timer tree. -Kees
T24gV2VkLCAyMDE3LTEwLTI1IGF0IDE2OjEwICswMjAwLCBLZWVzIENvb2sgd3JvdGU6DQo+IEhv d2V2ZXIsIG1haW50YWluZXJzOiBzb3JyeSB0byBzZW5kIHRoaXMgb25lIC0tIGl0IGNhbid0IGJl IG1lcmdlZA0KPiB5ZXQsIHRoaXMgdXNlcyB0aW1lcl9zZXR1cF9vbl9zdGFjaygpIHdoaWNoIGlz IG9ubHkgaW4gLW5leHQgcmlnaHQNCj4gbm93LiBJZiBpdCBsb29rcyBva2F5LCB0aG91Z2gsIEkg Y2FuIGNhcnJ5IHRoaXMgaW4gdGhlIHRpbWVyIHRyZWUuDQoNCkhlbGxvIEtlZXMsDQoNCkNhbiB5 b3UgaGF2ZSBhIGxvb2sgYXQgdGhlIGZvbGxvd2luZyBwYXRjaCBhbmQgcXVldWUgaXQgYmVmb3Jl IHlvdXINCnBhdGNoIGlmIHlvdSB0aGluayB0aGlzIHdvdWxkIGJlIHVzZWZ1bCAodGhpcyBwYXRj aCBoYXMgYmVlbiB0ZXN0ZWQpOg0KInRhcmdldC9pc2NzaTogU2ltcGxpZnkgdGltZXIgbWFuaXB1 bGF0aW9uIGNvZGUiDQooaHR0cHM6Ly93d3cuc3Bpbmljcy5uZXQvbGlzdHMvdGFyZ2V0LWRldmVs L21zZzE1Mzg1Lmh0bWwpLg0KDQpUaGFua3MsDQoNCkJhcnQu -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 25, 2017 at 5:03 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Wed, 2017-10-25 at 16:10 +0200, Kees Cook wrote: >> However, maintainers: sorry to send this one -- it can't be merged >> yet, this uses timer_setup_on_stack() which is only in -next right >> now. If it looks okay, though, I can carry this in the timer tree. > > Hello Kees, > > Can you have a look at the following patch and queue it before your > patch if you think this would be useful (this patch has been tested): > "target/iscsi: Simplify timer manipulation code" > (https://www.spinics.net/lists/target-devel/msg15385.html). This looks good, yeah. Can you resend it with me CCed? It doesn't look like it was CCed to lkml, so it's not easy to for me to add it to me tree. Thanks! -Kees
On Thu, 2017-10-26 at 10:24 +0200, Kees Cook wrote: > On Wed, Oct 25, 2017 at 5:03 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > > On Wed, 2017-10-25 at 16:10 +0200, Kees Cook wrote: > > > However, maintainers: sorry to send this one -- it can't be merged > > > yet, this uses timer_setup_on_stack() which is only in -next right > > > now. If it looks okay, though, I can carry this in the timer tree. > > > > Hello Kees, > > > > Can you have a look at the following patch and queue it before your > > patch if you think this would be useful (this patch has been tested): > > "target/iscsi: Simplify timer manipulation code" > > (https://www.spinics.net/lists/target-devel/msg15385.html). > > This looks good, yeah. Can you resend it with me CCed? It doesn't look > like it was CCed to lkml, so it's not easy to for me to add it to me > tree. OK, I will resend that patch as a reply to this e-mail. BTW, I just finished retesting that patch on top of v4.14-rc6. Bart.
On Thu, Oct 26, 2017 at 4:04 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Thu, 2017-10-26 at 10:24 +0200, Kees Cook wrote: >> On Wed, Oct 25, 2017 at 5:03 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: >> > On Wed, 2017-10-25 at 16:10 +0200, Kees Cook wrote: >> > > However, maintainers: sorry to send this one -- it can't be merged >> > > yet, this uses timer_setup_on_stack() which is only in -next right >> > > now. If it looks okay, though, I can carry this in the timer tree. >> > >> > Hello Kees, >> > >> > Can you have a look at the following patch and queue it before your >> > patch if you think this would be useful (this patch has been tested): >> > "target/iscsi: Simplify timer manipulation code" >> > (https://www.spinics.net/lists/target-devel/msg15385.html). >> >> This looks good, yeah. Can you resend it with me CCed? It doesn't look >> like it was CCed to lkml, so it's not easy to for me to add it to me >> tree. > > OK, I will resend that patch as a reply to this e-mail. BTW, I just finished > retesting that patch on top of v4.14-rc6. Thanks for the testing! -Kees
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c index 7fe2aa73cff6..4c9db2525d01 100644 --- a/drivers/target/iscsi/iscsi_target_erl0.c +++ b/drivers/target/iscsi/iscsi_target_erl0.c @@ -749,9 +749,9 @@ int iscsit_check_post_dataout( } } -static void iscsit_handle_time2retain_timeout(unsigned long data) +static void iscsit_handle_time2retain_timeout(struct timer_list *t) { - struct iscsi_session *sess = (struct iscsi_session *) data; + struct iscsi_session *sess = from_timer(sess, t, time2retain_timer); struct iscsi_portal_group *tpg = sess->tpg; struct se_portal_group *se_tpg = &tpg->tpg_se_tpg; @@ -809,11 +809,10 @@ void iscsit_start_time2retain_handler(struct iscsi_session *sess) pr_debug("Starting Time2Retain timer for %u seconds on" " SID: %u\n", sess->sess_ops->DefaultTime2Retain, sess->sid); - init_timer(&sess->time2retain_timer); + timer_setup(&sess->time2retain_timer, + iscsit_handle_time2retain_timeout, 0); sess->time2retain_timer.expires = (get_jiffies_64() + sess->sess_ops->DefaultTime2Retain * HZ); - sess->time2retain_timer.data = (unsigned long)sess; - sess->time2retain_timer.function = iscsit_handle_time2retain_timeout; sess->time2retain_timer_flags &= ~ISCSI_TF_STOP; sess->time2retain_timer_flags |= ISCSI_TF_RUNNING; add_timer(&sess->time2retain_timer); diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c index fe9b7f1e44ac..6655a01e6b94 100644 --- a/drivers/target/iscsi/iscsi_target_erl1.c +++ b/drivers/target/iscsi/iscsi_target_erl1.c @@ -1148,11 +1148,11 @@ static int iscsit_set_dataout_timeout_values( /* * NOTE: Called from interrupt (timer) context. */ -static void iscsit_handle_dataout_timeout(unsigned long data) +static void iscsit_handle_dataout_timeout(struct timer_list *t) { u32 pdu_length = 0, pdu_offset = 0; u32 r2t_length = 0, r2t_offset = 0; - struct iscsi_cmd *cmd = (struct iscsi_cmd *) data; + struct iscsi_cmd *cmd = from_timer(cmd, t, dataout_timer); struct iscsi_conn *conn = cmd->conn; struct iscsi_session *sess = NULL; struct iscsi_node_attrib *na; @@ -1264,10 +1264,8 @@ void iscsit_start_dataout_timer( pr_debug("Starting DataOUT timer for ITT: 0x%08x on" " CID: %hu.\n", cmd->init_task_tag, conn->cid); - init_timer(&cmd->dataout_timer); + timer_setup(&cmd->dataout_timer, iscsit_handle_dataout_timeout, 0); cmd->dataout_timer.expires = (get_jiffies_64() + na->dataout_timeout * HZ); - cmd->dataout_timer.data = (unsigned long)cmd; - cmd->dataout_timer.function = iscsit_handle_dataout_timeout; cmd->dataout_timer_flags &= ~ISCSI_TF_STOP; cmd->dataout_timer_flags |= ISCSI_TF_RUNNING; add_timer(&cmd->dataout_timer); diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index dc13afbd4c88..1825579b4eef 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -839,9 +839,9 @@ void iscsi_post_login_handler( iscsit_dec_conn_usage_count(conn); } -static void iscsi_handle_login_thread_timeout(unsigned long data) +static void iscsi_handle_login_thread_timeout(struct timer_list *t) { - struct iscsi_np *np = (struct iscsi_np *) data; + struct iscsi_np *np = from_timer(np, t, np_login_timer); spin_lock_bh(&np->np_thread_lock); pr_err("iSCSI Login timeout on Network Portal %pISpc\n", @@ -866,10 +866,8 @@ static void iscsi_start_login_thread_timer(struct iscsi_np *np) * point we do not have access to ISCSI_TPG_ATTRIB(tpg)->login_timeout */ spin_lock_bh(&np->np_thread_lock); - init_timer(&np->np_login_timer); + timer_setup(&np->np_login_timer, iscsi_handle_login_thread_timeout, 0); np->np_login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); - np->np_login_timer.data = (unsigned long)np; - np->np_login_timer.function = iscsi_handle_login_thread_timeout; np->np_login_timer_flags &= ~ISCSI_TF_STOP; np->np_login_timer_flags |= ISCSI_TF_RUNNING; add_timer(&np->np_login_timer); diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c index 7a6751fecd32..7d8d53a5a4c2 100644 --- a/drivers/target/iscsi/iscsi_target_nego.c +++ b/drivers/target/iscsi/iscsi_target_nego.c @@ -559,9 +559,15 @@ static void iscsi_target_login_drop(struct iscsi_conn *conn, struct iscsi_login iscsi_target_login_sess_out(conn, np, zero_tsih, true); } -static void iscsi_target_login_timeout(unsigned long data) +struct conn_timeout { + struct timer_list timer; + struct iscsi_conn *conn; +}; + +static void iscsi_target_login_timeout(struct timer_list *t) { - struct iscsi_conn *conn = (struct iscsi_conn *)data; + struct conn_timeout *timeout = from_timer(timeout, t, timer); + struct iscsi_conn *conn = timeout->conn; pr_debug("Entering iscsi_target_login_timeout >>>>>>>>>>>>>>>>>>>\n"); @@ -580,7 +586,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work) struct iscsi_np *np = login->np; struct iscsi_portal_group *tpg = conn->tpg; struct iscsi_tpg_np *tpg_np = conn->tpg_np; - struct timer_list login_timer; + struct conn_timeout timeout; int rc, zero_tsih = login->zero_tsih; bool state; @@ -618,15 +624,14 @@ static void iscsi_target_do_login_rx(struct work_struct *work) conn->login_kworker = current; allow_signal(SIGINT); - init_timer(&login_timer); - login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); - login_timer.data = (unsigned long)conn; - login_timer.function = iscsi_target_login_timeout; - add_timer(&login_timer); + timer_setup_on_stack(&timeout.timer, iscsi_target_login_timeout, 0); + timeout.timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ); + add_timer(&timeout.timer); pr_debug("Starting login_timer for %s/%d\n", current->comm, current->pid); rc = conn->conn_transport->iscsit_get_login_rx(conn, login); - del_timer_sync(&login_timer); + del_timer_sync(&timeout.timer); + destroy_timer_on_stack(&timeout.timer); flush_signals(current); conn->login_kworker = NULL; diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c index 1e36f83b5961..32aa06cf9a27 100644 --- a/drivers/target/iscsi/iscsi_target_util.c +++ b/drivers/target/iscsi/iscsi_target_util.c @@ -880,9 +880,9 @@ static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response) return 0; } -static void iscsit_handle_nopin_response_timeout(unsigned long data) +static void iscsit_handle_nopin_response_timeout(struct timer_list *t) { - struct iscsi_conn *conn = (struct iscsi_conn *) data; + struct iscsi_conn *conn = from_timer(conn, t, nopin_response_timer); iscsit_inc_conn_usage_count(conn); @@ -949,11 +949,10 @@ void iscsit_start_nopin_response_timer(struct iscsi_conn *conn) return; } - init_timer(&conn->nopin_response_timer); + timer_setup(&conn->nopin_response_timer, + iscsit_handle_nopin_response_timeout, 0); conn->nopin_response_timer.expires = (get_jiffies_64() + na->nopin_response_timeout * HZ); - conn->nopin_response_timer.data = (unsigned long)conn; - conn->nopin_response_timer.function = iscsit_handle_nopin_response_timeout; conn->nopin_response_timer_flags &= ~ISCSI_TF_STOP; conn->nopin_response_timer_flags |= ISCSI_TF_RUNNING; add_timer(&conn->nopin_response_timer); @@ -980,9 +979,9 @@ void iscsit_stop_nopin_response_timer(struct iscsi_conn *conn) spin_unlock_bh(&conn->nopin_timer_lock); } -static void iscsit_handle_nopin_timeout(unsigned long data) +static void iscsit_handle_nopin_timeout(struct timer_list *t) { - struct iscsi_conn *conn = (struct iscsi_conn *) data; + struct iscsi_conn *conn = from_timer(conn, t, nopin_timer); iscsit_inc_conn_usage_count(conn); @@ -1015,10 +1014,8 @@ void __iscsit_start_nopin_timer(struct iscsi_conn *conn) if (conn->nopin_timer_flags & ISCSI_TF_RUNNING) return; - init_timer(&conn->nopin_timer); + timer_setup(&conn->nopin_timer, iscsit_handle_nopin_timeout, 0); conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); - conn->nopin_timer.data = (unsigned long)conn; - conn->nopin_timer.function = iscsit_handle_nopin_timeout; conn->nopin_timer_flags &= ~ISCSI_TF_STOP; conn->nopin_timer_flags |= ISCSI_TF_RUNNING; add_timer(&conn->nopin_timer); @@ -1043,10 +1040,8 @@ void iscsit_start_nopin_timer(struct iscsi_conn *conn) return; } - init_timer(&conn->nopin_timer); + timer_setup(&conn->nopin_timer, iscsit_handle_nopin_timeout, 0); conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ); - conn->nopin_timer.data = (unsigned long)conn; - conn->nopin_timer.function = iscsit_handle_nopin_timeout; conn->nopin_timer_flags &= ~ISCSI_TF_STOP; conn->nopin_timer_flags |= ISCSI_TF_RUNNING; add_timer(&conn->nopin_timer);
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. Includes a fix for correcting an on-stack timer usage. Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org> Cc: Jiang Yi <jiangyilism@gmail.com> Cc: Varun Prakash <varun@chelsio.com> Cc: Bart Van Assche <bart.vanassche@sandisk.com> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-scsi@vger.kernel.org Cc: target-devel@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/target/iscsi/iscsi_target_erl0.c | 9 ++++----- drivers/target/iscsi/iscsi_target_erl1.c | 8 +++----- drivers/target/iscsi/iscsi_target_login.c | 8 +++----- drivers/target/iscsi/iscsi_target_nego.c | 23 ++++++++++++++--------- drivers/target/iscsi/iscsi_target_util.c | 21 ++++++++------------- 5 files changed, 32 insertions(+), 37 deletions(-)