FAILED: patch "[PATCH] can: isotp: fix timer drain order, wakeup handling and tx_gen" failed to apply to 6.18-stable tree
Subsystems (2)
[email protected]
maintainerThe patch below does not apply to the 6.18-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.18.y
git checkout FETCH_HEAD
git cherry-pick -x 050f010f920da17c1044a4f174766ad553e770b6
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2026080551-roundworm-petite-bf48@gregkh' --subject-prefix 'PATCH 6.18.y' 'HEAD^..'
Possible dependencies:
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 050f010f920da17c1044a4f174766ad553e770b6 Mon Sep 17 00:00:00 2001
From: Oliver Hartkopp <[email protected]>
Date: Fri, 24 Jul 2026 20:15:25 +0200
Subject: [PATCH] can: isotp: fix timer drain order, wakeup handling and tx_gen
ordering
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
net/can/isotp.c +230 -87
@@ -127,6 +127,15 @@ MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "#define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,
@@ -166,7 +175,8 @@ struct isotp_sock {u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;
@@ -177,6 +187,65 @@ static LIST_HEAD(isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk;
@@ -199,7 +268,7 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */
@@ -208,7 +277,7 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer)sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ;
@@ -372,20 +441,19 @@ static void isotp_send_cframe(struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ; if ((cf->len < ae + FC_CONTENT_SZ) ||
@@ -396,13 +464,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get static/dynamic communication params from first/every FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC || +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC || so->opt.flags &CAN_ISOTP_DYN_FC_PARMS ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];
@@ -426,13 +496,13 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->tx_gap =ktime_add_ns (so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );
@@ -447,14 +517,19 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ;
@@ -467,7 +542,7 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen,struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ;
@@ -501,7 +576,7 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);
@@ -555,7 +630,7 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )
@@ -573,7 +648,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */
@@ -588,11 +663,9 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */
@@ -613,7 +686,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++;
@@ -627,7 +700,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {
@@ -698,8 +771,10 @@ static void isotp_rcv(struct sk_buff *skb, void *data)if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }
@@ -794,6 +869,7 @@ static void isotp_send_cframe(struct isotp_sock *so)struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)
@@ -814,6 +890,9 @@ static void isotp_send_cframe(struct isotp_sock *so) csx->can_iif = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);
@@ -830,12 +909,15 @@ static void isotp_send_cframe(struct isotp_sock *so) skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );
@@ -887,7 +969,6 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)
@@ -899,32 +980,35 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock;
@@ -946,16 +1030,20 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )
@@ -965,14 +1053,14 @@ static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk);wake_up_interruptible (&so->wait);
@@ -1007,7 +1095,7 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer) HRTIMER_MODE_REL_SOFT);/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ;
@@ -1026,10 +1114,12 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim
@@ -1046,29 +1136,33 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ;
@@ -1127,6 +1221,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) csx->can_iif = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ;
@@ -1134,8 +1231,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {
@@ -1163,7 +1261,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */
@@ -1180,7 +1278,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;
@@ -1190,12 +1288,12 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */
@@ -1206,7 +1304,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);
@@ -1223,20 +1321,49 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) __func__, ERR_PTR(err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size;
@@ -1246,15 +1373,26 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);
@@ -1320,8 +1458,9 @@ static int isotp_release(struct socket *sock)/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its
@@ -1329,9 +1468,12 @@ static int isotp_release(struct socket *sock)* unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {
@@ -1447,7 +1589,8 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l* with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }
@@ -1481,7 +1624,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l isotp_rcv, sk, "isotp", sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
@@ -1847,7 +1990,7 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *wpoll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask;
Oliver Hartkopp
maintainerPATCH 1/1 can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
commit 050f010f920da17c1044a4f174766ad553e770b6 upstream.
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
--- net/can/isotp.c | 333 +++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 246 insertions(+), 87 deletions(-)
net/can/isotp.c +246 -87
@@ -126,6 +126,15 @@ MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "#define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,
@@ -165,7 +174,8 @@ struct isotp_sock {u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;
@@ -176,6 +186,81 @@ static LIST_HEAD(isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} + +/* can_set_skb_uid() (from commit d4fb6514ff8ed6912a71294e6b66a5d59ee88007, +* "can: use skb hash instead of private variable in headroom") is not +* available before mainline v7.0 - provide the same non-zero unique +* loopback echo identifier locally instead. +*/ +static atomic_t isotp_skb_uid_seq =ATOMIC_INIT (0 ); + +static void isotp_set_skb_uid (struct sk_buff *skb) +{ +/* create non-zero unique skb identifier together with *skb */ +while (!(skb->hash)) + skb->hash =atomic_inc_return (&isotp_skb_uid_seq); + + skb->sw_hash =1 ; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk;
@@ -198,7 +283,7 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */
@@ -207,7 +292,7 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer)sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ;
@@ -366,20 +451,19 @@ static void isotp_send_cframe(struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ; if ((cf->len < ae + FC_CONTENT_SZ) ||
@@ -390,13 +474,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get static/dynamic communication params from first/every FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC || +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC || so->opt.flags &CAN_ISOTP_DYN_FC_PARMS ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];
@@ -420,13 +506,13 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->tx_gap =ktime_add_ns (so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );
@@ -441,14 +527,19 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ;
@@ -461,7 +552,7 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen,struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ;
@@ -495,7 +586,7 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);
@@ -549,7 +640,7 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )
@@ -567,7 +658,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */
@@ -582,11 +673,9 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */
@@ -607,7 +696,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++;
@@ -621,7 +710,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {
@@ -692,8 +781,10 @@ static void isotp_rcv(struct sk_buff *skb, void *data)if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }
@@ -787,6 +878,7 @@ static void isotp_send_cframe(struct isotp_sock *so)struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)
@@ -802,6 +894,9 @@ static void isotp_send_cframe(struct isotp_sock *so)can_skb_prv (skb)->ifindex = dev->ifindex;can_skb_prv (skb)->skbcnt =0 ; +/* set uid in tx skb to identify CF echo frames */ +isotp_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);
@@ -818,12 +913,15 @@ static void isotp_send_cframe(struct isotp_sock *so) skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );
@@ -875,7 +973,6 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)
@@ -887,32 +984,35 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock;
@@ -934,16 +1034,20 @@ out_unlock:spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )
@@ -953,14 +1057,14 @@ static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk);wake_up_interruptible (&so->wait);
@@ -995,7 +1099,7 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer) HRTIMER_MODE_REL_SOFT);/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ;
@@ -1013,10 +1117,12 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim
@@ -1033,29 +1139,33 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ;
@@ -1108,6 +1218,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)can_skb_prv (skb)->ifindex = dev->ifindex;can_skb_prv (skb)->skbcnt =0 ; +/* set uid in tx skb to identify CF echo frames */ +isotp_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ;
@@ -1115,8 +1228,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {
@@ -1144,7 +1258,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */
@@ -1161,7 +1275,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;
@@ -1171,12 +1285,12 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */
@@ -1187,7 +1301,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);
@@ -1204,20 +1318,49 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) __func__, ERR_PTR(err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size;
@@ -1227,15 +1370,26 @@ err_out_drop:spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);
@@ -1301,8 +1455,9 @@ static int isotp_release(struct socket *sock)/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its
@@ -1310,9 +1465,12 @@ static int isotp_release(struct socket *sock)* unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {
@@ -1428,7 +1586,8 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)* with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }
@@ -1462,7 +1621,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) isotp_rcv, sk, "isotp", sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
@@ -1828,7 +1987,7 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *wpoll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask;
Signature · 1 line
2.43.0
Sasha Levin
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
The skb->hash echo tag doesn't work here. isotp_set_skb_uid() is only called from the TX paths, never from isotp_send_fc(), so FC loopback echoes arrive with skb->hash == 0 and alias the so->cfecho == 0 "nothing pending" sentinel. Upstream is fine because d4fb6514ff8e stamps the UID in can_receive(), but that commit isn't on 6.18.
Signature · 2 lines
Thanks, Sasha
Oliver Hartkopp
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
Quoted 6 lines · Sasha Levin
The
skb->hashecho tag doesn't work here.isotp_set_skb_uid()is only called from the TX paths, never fromisotp_send_fc(), so FC loopback echoes arrive withskb->hash== 0 and alias theso->cfecho== 0 "nothing pending" sentinel. Upstream is fine becaused4fb6514ff8estamps the UID incan_receive(), but that commit isn't on 6.18.
Ouch! Thanks for catching this!!
I just focused on isotp.c - my bad.
What would be your suggestion to solve this issue?
1. create a 2-patch series which applies d4fb6514ff8e first?
2. turn it back to the original if (so->cfecho != *(u32 *)cf->data) code?
The latter was tagged by sashiko-bot to be not safe enough.
Many thanks,
Oliver
Sasha Levin
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
Quoted 12 lines · Oliver Hartkopp
Quoted 6 lines · Sasha Levin
The
skb->hashecho tag doesn't work here.isotp_set_skb_uid()is only called from the TX paths, never fromisotp_send_fc(), so FC loopback echoes arrive withskb->hash== 0 and alias theso->cfecho== 0 "nothing pending" sentinel. Upstream is fine becaused4fb6514ff8estamps the UID incan_receive(), but that commit isn't on 6.18.Ouch! Thanks for catching this!!
Quoted 8 lines
I just focused on
isotp.c- my bad.What would be your suggestion to solve this issue?
1. create a 2-patch series which applies
d4fb6514ff8efirst?
2. turn it back to the original if (so->cfecho!= *(u32 *)cf->data) code?The latter was tagged by sashiko-bot to be not safe enough.
I think that just a 2 patch series would make the most sense. Thanks!
Signature · 2 lines
Thanks, Sasha
Oliver Hartkopp
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
Quoted 15 lines · Sasha Levin
Quoted 14 lines · Oliver Hartkopp
Quoted 6 lines · Sasha Levin
The
skb->hashecho tag doesn't work here.isotp_set_skb_uid()is only called from the TX paths, never fromisotp_send_fc(), so FC loopback echoes arrive withskb->hash== 0 and alias theso->cfecho== 0 "nothing pending" sentinel. Upstream is fine becaused4fb6514ff8estamps the UID incan_receive(), but that commit isn't on 6.18.Ouch! Thanks for catching this!!
I just focused on
isotp.c- my bad.
Quoted 9 lines
Quoted 6 lines
What would be your suggestion to solve this issue?
1. create a 2-patch series which applies
d4fb6514ff8efirst?
2. turn it back to the original if (so->cfecho!= *(u32 *)cf->data) code?The latter was tagged by sashiko-bot to be not safe enough.
I think that just a 2 patch series would make the most sense. Thanks!
I just posted the v2 patch series for 6.6.y , 6.12.y and 6.18.y
Btw. I double checked that the former integration would have been working too, as we have a special case for those echo skbs, that get their uid already in isotp.c and not in can_receive() to detect the echo of self generated skbs.
Anyway the solution with two patches is much better as it makes the isotp.c code more look like the mainline HEAD for ongoing maintenance.
I'll start the work for 5.10.y 5.15.y 6.1.y too - looking forward to the FAIL email to get the --in-reply-to Tag ;-)
Many thanks and best regards,
Oliver
Oliver Hartkopp
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
Hi Sasha,
Quoted 4 lines · Oliver Hartkopp
Quoted 2 lines
I think that just a 2 patch series would make the most sense. Thanks!
I just posted the v2 patch series for 6.6.y , 6.12.y and 6.18.y
upstream commit 050f010f920da17c1044a4f174766ad553e770b6 is now integrated in 6.6.y , 6.12.y and 6.18.y
But I didn't get a FAIL notice for 5.10.y 5.15.y 6.1.y so far.
Many thanks,
Oliver
Sasha Levin
maintainerRe: [PATCH 6.18.y] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
Quoted 4 lines · Oliver Hartkopp
upstream commit
050f010f920da17c1044a4f174766ad553e770b6is now integrated in 6.6.y , 6.12.y and 6.18.yBut I didn't get a FAIL notice for 5.10.y 5.15.y 6.1.y so far.
No notice is coming for those, sorry - cf070fe33bfb reached 6.1.y, 5.15.y and 5.10.y by a different route, so 050f010f920d was never auto-tried against them and nothing generated a failure mail.
The gap is real though: all three carry cf070fe33bfb without its follow-up. Could you send the same v2-style 2-patch series (d4fb6514ff8e + 050f010f920d) for 6.1.y, 5.15.y and 5.10.y, threaded to this mail?
Signature · 2 lines
Thanks, Sasha
Oliver Hartkopp
maintainerPATCH 1/2 can: use skb hash instead of private variable in headroom
commit d4fb6514ff8ed6912a71294e6b66a5d59ee88007 upstream.
The can_skb_priv::skbcnt variable is used to identify CAN skbs in the RX path analogue to the skb->hash.
As the skb hash is not filled in CAN skbs move the private skbcnt value to skb->hash and set skb->sw_hash accordingly. The skb->hash is a value used for RPS to identify skbs. Use it as intended.
--- drivers/net/can/dev/dev.c | 2 -- drivers/net/can/slcan.c | 1 - include/linux/can/core.h | 1 + include/linux/can/dev.h | 1 - include/linux/can/skb.h | 2 -- net/can/af_can.c | 14 +++++++++++--- net/can/bcm.c | 2 -- net/can/isotp.c | 3 --- net/can/j1939/socket.c | 1 - net/can/j1939/transport.c | 2 -- net/can/raw.c | 7 +++---- 11 files changed, 15 insertions(+), 21 deletions(-)
drivers/net/can/dev/dev.c +0 -2
@@ -692,11 +692,10 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; *cf =skb_put_zero (skb,sizeof (struct can_frame ));return skb; }
@@ -720,11 +719,10 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; *cfd =skb_put_zero (skb,sizeof (struct canfd_frame ));return skb; }
drivers/net/can/slcan.c +0 -1
@@ -211,11 +211,10 @@ static void slc_bump(struct slcan *sl) skb->pkt_type =PACKET_BROADCAST ; skb->ip_summed =CHECKSUM_UNNECESSARY ;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = sl->dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, &cf,sizeof (struct can_frame )); sl->dev->stats.rx_packets++; sl->dev->stats.rx_bytes += cf.can_dlc;
include/linux/can/core.h +1 -0
@@ -56,8 +56,9 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), void *data);extern int can_send (struct sk_buff *skb,int loop); +void can_set_skb_uid (struct sk_buff *skb);void can_sock_destruct (struct sock *sk);#endif /* !_CAN_CORE_H */
include/linux/can/dev.h +0 -1
@@ -117,11 +117,10 @@ static inline bool can_skb_headroom_valid(struct net_device *dev,/* af_packet does not apply CAN skb specific settings */ if (skb->ip_summed ==CHECKSUM_NONE ) {/* init headroom */ can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->ip_summed =CHECKSUM_UNNECESSARY ;/* perform proper loopback on capable devices */ if (dev->flags &IFF_ECHO )
include/linux/can/skb.h +0 -2
@@ -26,16 +26,14 @@*/ /** * struct can_skb_priv - private additional data inside CAN sk_buffs * @ifindex: ifindex of the first interface the CAN frame appeared on -* @skbcnt: atomic counter to have an unique id together with skb pointer * @cf: align to the following CAN frame at skb->data */ struct can_skb_priv {int ifindex; -int skbcnt;struct can_frame cf[]; };static inline struct can_skb_priv *can_skb_prv (struct sk_buff *skb) {
net/can/af_can.c +11 -3
@@ -639,10 +639,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf }return matches; } +void can_set_skb_uid (struct sk_buff *skb) +{ +/* create non-zero unique skb identifier together with *skb */ +while (!(skb->hash)) + skb->hash =atomic_inc_return (&skbcounter); + + skb->sw_hash =1 ; +} +EXPORT_SYMBOL (can_set_skb_uid); +static void can_receive (struct sk_buff *skb,struct net_device *dev) {struct can_dev_rcv_lists *dev_rcv_lists;struct net *net =dev_net (dev);struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
@@ -650,13 +660,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)/* update statistics */ atomic_long_inc (&pkg_stats->rx_frames);atomic_long_inc (&pkg_stats->rx_frames_delta); -/* create non-zero unique skb identifier together with *skb */ -while (!(can_skb_prv (skb)->skbcnt)) -can_skb_prv (skb)->skbcnt =atomic_inc_return (&skbcounter); +can_set_skb_uid (skb);rcu_read_lock ();/* deliver the packet to sockets listening on all devices */ matches =can_rcv_filter (net->can.rx_alldev_list, skb);
net/can/bcm.c +0 -2
@@ -336,11 +336,10 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)if (!skb) goto out;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, cf, op->cfsiz);/* send with loopback */ skb->dev = dev;
@@ -1575,11 +1574,10 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,kfree_skb (skb);return -ENODEV ; }can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->dev = dev;can_skb_set_owner (skb, sk); err =can_send (skb,1 );/* send with loopback */ dev_put (dev);
net/can/isotp.c +0 -3
@@ -216,11 +216,10 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)return 1 ; }can_skb_reserve (nskb);can_skb_prv (nskb)->ifindex = dev->ifindex; -can_skb_prv (nskb)->skbcnt =0 ; nskb->dev = dev;can_skb_set_owner (nskb, sk); ncf = (struct canfd_frame *)nskb->data;skb_put_zero (nskb, so->ll.mtu);
@@ -771,11 +770,10 @@ static void isotp_send_cframe(struct isotp_sock *so)return ; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */
@@ -1067,11 +1065,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto err_out_drop; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;
net/can/j1939/socket.c +0 -1
@@ -887,11 +887,10 @@ static struct sk_buff *j1939_sk_alloc_skb(struct net_device *ndev,if (!skb) goto failure;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_reserve (skb, offsetof(struct can_frame , data)); ret =memcpy_from_msg (skb_put (skb, size), msg, size);if (ret <0 ) goto free_skb;
net/can/j1939/transport.c +0 -2
@@ -606,11 +606,10 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,return ERR_PTR (-ENOMEM ); skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* reserve CAN header */ skb_reserve (skb, offsetof(struct can_frame , data));/* skb->cb must be large enough to hold a j1939_sk_buff_cb structure */ BUILD_BUG_ON (sizeof (skb->cb) <sizeof (*re_skcb));
@@ -1533,11 +1532,10 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,return NULL ; skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skcb =j1939_skb_to_cb (skb);memcpy (skcb, rel_skcb,sizeof (*skcb)); session =j1939_session_new (priv, skb, size);if (!session) {
net/can/raw.c +3 -4
@@ -72,12 +72,12 @@ MODULE_ALIAS("can-proto-1");* list containing only one item. This common case is optimized by * storing the single filter in dfilter, to avoid using dynamic memory. */ struct uniqframe { -int skbcnt;const struct sk_buff *skb; +u32 hash;unsigned int join_rx_count; };struct raw_sock {struct sock sk;
@@ -133,22 +133,22 @@ static void raw_rcv(struct sk_buff *oskb, void *data)if (!ro->fd_frames && oskb->len !=CAN_MTU )return ;/* eliminate multiple filter matches for the same skb */ if (this_cpu_ptr (ro->uniq)->skb == oskb && -this_cpu_ptr (ro->uniq)->skbcnt ==can_skb_prv (oskb)->skbcnt) { +this_cpu_ptr (ro->uniq)->hash == oskb->hash) {if (ro->join_filters) {this_cpu_inc (ro->uniq->join_rx_count);/* drop frame until all enabled filters matched */ if (this_cpu_ptr (ro->uniq)->join_rx_count < ro->count)return ; }else {return ; } }else {this_cpu_ptr (ro->uniq)->skb = oskb; -this_cpu_ptr (ro->uniq)->skbcnt =can_skb_prv (oskb)->skbcnt; +this_cpu_ptr (ro->uniq)->hash = oskb->hash;this_cpu_ptr (ro->uniq)->join_rx_count =1 ;/* drop first frame to check all enabled filters? */ if (ro->join_filters && ro->count >1 )return ; }
@@ -810,11 +810,10 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (!skb) goto put_dev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; err =memcpy_from_msg (skb_put (skb, size), msg, size);if (err <0 ) goto free_skb;
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH 1/2 can: use skb hash instead of private variable in headroom
commit d4fb6514ff8ed6912a71294e6b66a5d59ee88007 upstream.
The can_skb_priv::skbcnt variable is used to identify CAN skbs in the RX path analogue to the skb->hash.
As the skb hash is not filled in CAN skbs move the private skbcnt value to skb->hash and set skb->sw_hash accordingly. The skb->hash is a value used for RPS to identify skbs. Use it as intended.
--- drivers/net/can/dev/skb.c | 2 -- include/linux/can/core.h | 1 + include/linux/can/skb.h | 2 -- net/can/af_can.c | 14 +++++++++++--- net/can/bcm.c | 2 -- net/can/isotp.c | 3 --- net/can/j1939/socket.c | 1 - net/can/j1939/transport.c | 2 -- net/can/raw.c | 7 +++---- 9 files changed, 15 insertions(+), 19 deletions(-)
drivers/net/can/dev/skb.c +0 -2
@@ -200,11 +200,10 @@ static void init_can_skb_reserve(struct sk_buff *skb)skb_reset_mac_header (skb);skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb); -can_skb_prv (skb)->skbcnt =0 ; }struct sk_buff *alloc_can_skb (struct net_device *dev,struct can_frame **cf) {struct sk_buff *skb;
@@ -310,11 +309,10 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)/* af_packet does not apply CAN skb specific settings */ if (skb->ip_summed ==CHECKSUM_NONE ) {/* init headroom */ can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->ip_summed =CHECKSUM_UNNECESSARY ;/* perform proper loopback on capable devices */ if (dev->flags &IFF_ECHO )
include/linux/can/core.h +1 -0
@@ -56,8 +56,9 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), void *data);extern int can_send (struct sk_buff *skb,int loop); +void can_set_skb_uid (struct sk_buff *skb);void can_sock_destruct (struct sock *sk);#endif /* !_CAN_CORE_H */
include/linux/can/skb.h +0 -2
@@ -47,17 +47,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);*/ /** * struct can_skb_priv - private additional data inside CAN sk_buffs * @ifindex: ifindex of the first interface the CAN frame appeared on -* @skbcnt: atomic counter to have an unique id together with skb pointer * @frame_len: length of CAN frame in data link layer * @cf: align to the following CAN frame at skb->data */ struct can_skb_priv {int ifindex; -int skbcnt;unsigned int frame_len;struct can_frame cf[]; };static inline struct can_skb_priv *can_skb_prv (struct sk_buff *skb)
net/can/af_can.c +11 -3
@@ -638,10 +638,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf }return matches; } +void can_set_skb_uid (struct sk_buff *skb) +{ +/* create non-zero unique skb identifier together with *skb */ +while (!(skb->hash)) + skb->hash =atomic_inc_return (&skbcounter); + + skb->sw_hash =1 ; +} +EXPORT_SYMBOL (can_set_skb_uid); +static void can_receive (struct sk_buff *skb,struct net_device *dev) {struct can_dev_rcv_lists *dev_rcv_lists;struct net *net =dev_net (dev);struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
@@ -649,13 +659,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)/* update statistics */ atomic_long_inc (&pkg_stats->rx_frames);atomic_long_inc (&pkg_stats->rx_frames_delta); -/* create non-zero unique skb identifier together with *skb */ -while (!(can_skb_prv (skb)->skbcnt)) -can_skb_prv (skb)->skbcnt =atomic_inc_return (&skbcounter); +can_set_skb_uid (skb);rcu_read_lock ();/* deliver the packet to sockets listening on all devices */ matches =can_rcv_filter (net->can.rx_alldev_list, skb);
net/can/bcm.c +0 -2
@@ -336,11 +336,10 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)if (!skb) goto out;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, cf, op->cfsiz);/* send with loopback */ skb->dev = dev;
@@ -1580,11 +1579,10 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,kfree_skb (skb);return -ENODEV ; }can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->dev = dev;can_skb_set_owner (skb, sk); err =can_send (skb,1 );/* send with loopback */ dev_put (dev);
net/can/isotp.c +0 -3
@@ -216,11 +216,10 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)return 1 ; }can_skb_reserve (nskb);can_skb_prv (nskb)->ifindex = dev->ifindex; -can_skb_prv (nskb)->skbcnt =0 ; nskb->dev = dev;can_skb_set_owner (nskb, sk); ncf = (struct canfd_frame *)nskb->data;skb_put_zero (nskb, so->ll.mtu);
@@ -771,11 +770,10 @@ static void isotp_send_cframe(struct isotp_sock *so)return ; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */
@@ -1067,11 +1065,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto err_out_drop; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;
net/can/j1939/socket.c +0 -1
@@ -887,11 +887,10 @@ static struct sk_buff *j1939_sk_alloc_skb(struct net_device *ndev,if (!skb) goto failure;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_reserve (skb, offsetof(struct can_frame , data)); ret =memcpy_from_msg (skb_put (skb, size), msg, size);if (ret <0 ) goto free_skb;
net/can/j1939/transport.c +0 -2
@@ -611,11 +611,10 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,return ERR_PTR (-ENOMEM ); skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* reserve CAN header */ skb_reserve (skb, offsetof(struct can_frame , data));/* skb->cb must be large enough to hold a j1939_sk_buff_cb structure */ BUILD_BUG_ON (sizeof (skb->cb) <sizeof (*re_skcb));
@@ -1551,11 +1550,10 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,return NULL ; skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skcb =j1939_skb_to_cb (skb);memcpy (skcb, rel_skcb,sizeof (*skcb)); session =j1939_session_new (priv, skb, size);if (!session) {
net/can/raw.c +3 -4
@@ -73,12 +73,12 @@ MODULE_ALIAS("can-proto-1");* list containing only one item. This common case is optimized by * storing the single filter in dfilter, to avoid using dynamic memory. */ struct uniqframe { -int skbcnt;const struct sk_buff *skb; +u32 hash;unsigned int join_rx_count; };struct raw_sock {struct sock sk;
@@ -138,21 +138,21 @@ static void raw_rcv(struct sk_buff *oskb, void *data) (!ro->xl_frames && can_is_canxl_skb(oskb))) return;/* eliminate multiple filter matches for the same skb */ if (this_cpu_ptr (ro->uniq)->skb == oskb && -this_cpu_ptr (ro->uniq)->skbcnt ==can_skb_prv (oskb)->skbcnt) { +this_cpu_ptr (ro->uniq)->hash == oskb->hash) {if (!ro->join_filters)return ;this_cpu_inc (ro->uniq->join_rx_count);/* drop frame until all enabled filters matched */ if (this_cpu_ptr (ro->uniq)->join_rx_count < ro->count)return ; }else {this_cpu_ptr (ro->uniq)->skb = oskb; -this_cpu_ptr (ro->uniq)->skbcnt =can_skb_prv (oskb)->skbcnt; +this_cpu_ptr (ro->uniq)->hash = oskb->hash;this_cpu_ptr (ro->uniq)->join_rx_count =1 ;/* drop first frame to check all enabled filters? */ if (ro->join_filters && ro->count >1 )return ; }
@@ -868,11 +868,10 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (!skb) goto put_dev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* fill the skb before testing for valid CAN frames */ err =memcpy_from_msg (skb_put (skb, size), msg, size);if (err <0 ) goto free_skb;
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH 1/2 can: use skb hash instead of private variable in headroom
In reply to Oliver Hartkoppcommit d4fb6514ff8ed6912a71294e6b66a5d59ee88007 upstream.
The can_skb_priv::skbcnt variable is used to identify CAN skbs in the RX path analogue to the skb->hash.
As the skb hash is not filled in CAN skbs move the private skbcnt value to skb->hash and set skb->sw_hash accordingly. The skb->hash is a value used for RPS to identify skbs. Use it as intended.
--- drivers/net/can/dev/skb.c | 2 -- drivers/net/can/slcan.c | 1 - include/linux/can/core.h | 1 + include/linux/can/skb.h | 3 --- net/can/af_can.c | 14 +++++++++++--- net/can/bcm.c | 2 -- net/can/isotp.c | 3 --- net/can/j1939/socket.c | 1 - net/can/j1939/transport.c | 2 -- net/can/raw.c | 7 +++---- 10 files changed, 15 insertions(+), 21 deletions(-)
drivers/net/can/dev/skb.c +0 -2
@@ -201,11 +201,10 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; *cf =skb_put_zero (skb,sizeof (struct can_frame ));return skb; }
@@ -232,11 +231,10 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; *cfd =skb_put_zero (skb,sizeof (struct canfd_frame ));return skb; }
drivers/net/can/slcan.c +0 -1
@@ -211,11 +211,10 @@ static void slc_bump(struct slcan *sl) skb->pkt_type =PACKET_BROADCAST ; skb->ip_summed =CHECKSUM_UNNECESSARY ;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = sl->dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, &cf,sizeof (struct can_frame )); sl->dev->stats.rx_packets++; sl->dev->stats.rx_bytes += cf.len;
include/linux/can/core.h +1 -0
@@ -56,8 +56,9 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), void *data);extern int can_send (struct sk_buff *skb,int loop); +void can_set_skb_uid (struct sk_buff *skb);void can_sock_destruct (struct sock *sk);#endif /* !_CAN_CORE_H */
include/linux/can/skb.h +0 -3
@@ -41,17 +41,15 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev,*/ /** * struct can_skb_priv - private additional data inside CAN sk_buffs * @ifindex: ifindex of the first interface the CAN frame appeared on -* @skbcnt: atomic counter to have an unique id together with skb pointer * @frame_len: length of CAN frame in data link layer * @cf: align to the following CAN frame at skb->data */ struct can_skb_priv {int ifindex; -int skbcnt;unsigned int frame_len;struct can_frame cf[]; };static inline struct can_skb_priv *can_skb_prv (struct sk_buff *skb)
@@ -105,11 +103,10 @@ static inline bool can_skb_headroom_valid(struct net_device *dev,/* af_packet does not apply CAN skb specific settings */ if (skb->ip_summed ==CHECKSUM_NONE ) {/* init headroom */ can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->ip_summed =CHECKSUM_UNNECESSARY ;/* perform proper loopback on capable devices */ if (dev->flags &IFF_ECHO )
net/can/af_can.c +11 -3
@@ -639,10 +639,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf }return matches; } +void can_set_skb_uid (struct sk_buff *skb) +{ +/* create non-zero unique skb identifier together with *skb */ +while (!(skb->hash)) + skb->hash =atomic_inc_return (&skbcounter); + + skb->sw_hash =1 ; +} +EXPORT_SYMBOL (can_set_skb_uid); +static void can_receive (struct sk_buff *skb,struct net_device *dev) {struct can_dev_rcv_lists *dev_rcv_lists;struct net *net =dev_net (dev);struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
@@ -650,13 +660,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)/* update statistics */ atomic_long_inc (&pkg_stats->rx_frames);atomic_long_inc (&pkg_stats->rx_frames_delta); -/* create non-zero unique skb identifier together with *skb */ -while (!(can_skb_prv (skb)->skbcnt)) -can_skb_prv (skb)->skbcnt =atomic_inc_return (&skbcounter); +can_set_skb_uid (skb);rcu_read_lock ();/* deliver the packet to sockets listening on all devices */ matches =can_rcv_filter (net->can.rx_alldev_list, skb);
net/can/bcm.c +0 -2
@@ -336,11 +336,10 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)if (!skb) goto out;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, cf, op->cfsiz);/* send with loopback */ skb->dev = dev;
@@ -1575,11 +1574,10 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,kfree_skb (skb);return -ENODEV ; }can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->dev = dev;can_skb_set_owner (skb, sk); err =can_send (skb,1 );/* send with loopback */ dev_put (dev);
net/can/isotp.c +0 -3
@@ -216,11 +216,10 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)return 1 ; }can_skb_reserve (nskb);can_skb_prv (nskb)->ifindex = dev->ifindex; -can_skb_prv (nskb)->skbcnt =0 ; nskb->dev = dev;can_skb_set_owner (nskb, sk); ncf = (struct canfd_frame *)nskb->data;skb_put_zero (nskb, so->ll.mtu);
@@ -771,11 +770,10 @@ static void isotp_send_cframe(struct isotp_sock *so)return ; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */
@@ -1067,11 +1065,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto err_out_drop; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;
net/can/j1939/socket.c +0 -1
@@ -887,11 +887,10 @@ static struct sk_buff *j1939_sk_alloc_skb(struct net_device *ndev,if (!skb) goto failure;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_reserve (skb, offsetof(struct can_frame , data)); ret =memcpy_from_msg (skb_put (skb, size), msg, size);if (ret <0 ) goto free_skb;
net/can/j1939/transport.c +0 -2
@@ -610,11 +610,10 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,return ERR_PTR (-ENOMEM ); skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* reserve CAN header */ skb_reserve (skb, offsetof(struct can_frame , data));/* skb->cb must be large enough to hold a j1939_sk_buff_cb structure */ BUILD_BUG_ON (sizeof (skb->cb) <sizeof (*re_skcb));
@@ -1549,11 +1548,10 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,return NULL ; skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skcb =j1939_skb_to_cb (skb);memcpy (skcb, rel_skcb,sizeof (*skcb)); session =j1939_session_new (priv, skb, size);if (!session) {
net/can/raw.c +3 -4
@@ -72,12 +72,12 @@ MODULE_ALIAS("can-proto-1");* list containing only one item. This common case is optimized by * storing the single filter in dfilter, to avoid using dynamic memory. */ struct uniqframe { -int skbcnt;const struct sk_buff *skb; +u32 hash;unsigned int join_rx_count; };struct raw_sock {struct sock sk;
@@ -134,22 +134,22 @@ static void raw_rcv(struct sk_buff *oskb, void *data)if (!ro->fd_frames && oskb->len !=CAN_MTU )return ;/* eliminate multiple filter matches for the same skb */ if (this_cpu_ptr (ro->uniq)->skb == oskb && -this_cpu_ptr (ro->uniq)->skbcnt ==can_skb_prv (oskb)->skbcnt) { +this_cpu_ptr (ro->uniq)->hash == oskb->hash) {if (ro->join_filters) {this_cpu_inc (ro->uniq->join_rx_count);/* drop frame until all enabled filters matched */ if (this_cpu_ptr (ro->uniq)->join_rx_count < ro->count)return ; }else {return ; } }else {this_cpu_ptr (ro->uniq)->skb = oskb; -this_cpu_ptr (ro->uniq)->skbcnt =can_skb_prv (oskb)->skbcnt; +this_cpu_ptr (ro->uniq)->hash = oskb->hash;this_cpu_ptr (ro->uniq)->join_rx_count =1 ;/* drop first frame to check all enabled filters? */ if (ro->join_filters && ro->count >1 )return ; }
@@ -827,11 +827,10 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (!skb) goto put_dev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; err =memcpy_from_msg (skb_put (skb, size), msg, size);if (err <0 ) goto free_skb;
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH 2/2 can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
In reply to Oliver Hartkoppcommit 050f010f920da17c1044a4f174766ad553e770b6 upstream.
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
--- net/can/isotp.c | 317 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 230 insertions(+), 87 deletions(-)
net/can/isotp.c +230 -87
@@ -112,10 +112,19 @@ MODULE_ALIAS("can-proto-6");#define ISOTP_FC_OVFLW 2/* overflow */ #define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,ISOTP_WAIT_FC ,ISOTP_WAIT_DATA ,
@@ -148,21 +157,81 @@ struct isotp_sock {struct can_isotp_ll_options ll;u32 frame_txtime;u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;spinlock_t rx_lock;/* protect single thread state machine */ };static LIST_HEAD (isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk; }
@@ -181,20 +250,20 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) { struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */ sk->sk_err =ETIMEDOUT ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ; }
@@ -347,41 +416,42 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,static void isotp_send_cframe (struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ;if ((cf->len < ae +FC_CONTENT_SZ ) || ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, ae +FC_CONTENT_SZ , so->opt.rxpad_content))) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get communication parameters only from the first FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC ) { +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];/* fix wrong STmin values according spec */ if (so->txfc.stmin > 0x7F &&
@@ -400,17 +470,17 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->txfc.stmin * 1000000); else so->tx_gap = ktime_add_ns(so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );isotp_send_cframe (so);break ;
@@ -421,18 +491,23 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) HRTIMER_MODE_REL_SOFT);break ;case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ; }
@@ -441,11 +516,11 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ; if ((so->opt.flags & ISOTP_CHECK_PADDING) &&
@@ -475,11 +550,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int i;int off;int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);/* the first frame has to use the entire frame up to LL_DL length */
@@ -519,11 +594,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)for (i = ae + ff_pci_sz; i < so->rx.ll_dl; i++) so->rx.buf[so->rx.idx++] = cf->data[i];/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )return 0 ;
@@ -537,11 +612,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */ if (so->opt.flags &CAN_ISOTP_FORCE_RXSTMIN ) { if (ktime_to_ns(ktime_sub(skb->tstamp, so->lastrxcf_tstamp)) <
@@ -552,15 +627,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, }hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */ if (cf->len > so->rx.ll_dl)return 1 ;
@@ -577,11 +650,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, sk->sk_err =EILSEQ ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++; so->rx.sn %=16 ;
@@ -591,11 +664,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,break ; }if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;
@@ -662,12 +735,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data)*/ spin_lock (&so->rx_lock);if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }switch (n_pci_type) {case N_PCI_FC :
@@ -757,10 +832,11 @@ static void isotp_send_cframe(struct isotp_sock *so)struct sk_buff *skb;struct net_device *dev;struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)return ;
@@ -771,10 +847,13 @@ static void isotp_send_cframe(struct isotp_sock *so) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */ isotp_fill_dataframe (cf, so, ae,0 );
@@ -787,16 +866,19 @@ static void isotp_send_cframe(struct isotp_sock *so) cf->flags = so->ll.tx_flags; skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );if (can_send_ret) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
@@ -844,11 +926,10 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,static void isotp_rcv_echo (struct sk_buff *skb,void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)return ;
@@ -856,36 +937,39 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)* (no isotp_rcv() caller here), so take it ourselves */ spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock; }
@@ -903,37 +987,41 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) out_unlock:spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )return HRTIMER_NORESTART ;/* only claim the timeout if the state is still unchanged */ if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk);wake_up_interruptible (&so->wait);return HRTIMER_NORESTART ; }
@@ -964,11 +1052,11 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)/* start echo timeout handling and cover below protocol error */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ; }
@@ -982,14 +1070,16 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ;int wait_tx_done = (so->opt.flags &CAN_ISOTP_WAIT_TX_DONE ) ?1 :0 ;s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim * with the RX path and with sendmsg()'s own error paths below, so * none of them can ever see a transfer mid-claim
@@ -1002,33 +1092,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* we do not support multiple buffers - for now */ if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ; goto err_out_drop; }
@@ -1066,19 +1160,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {/* The message size generally fits into a SingleFrame - good. *
@@ -1102,11 +1200,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[SF_PCI_SZ4 + ae] = size; else cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */ isotp_create_fframe (cf, so, ae);
@@ -1119,37 +1217,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* disable wait for FCs due to activated block size */ so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;/* start timeout for FC */ hrtimer_sec =ISOTP_FC_TIMEOUT ; tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */ spin_unlock_bh (&so->rx_lock);kfree_skb (skb);dev_put (dev);wake_up_interruptible (&so->wait);return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);/* send the first or only CAN frame */
@@ -1162,42 +1260,82 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (err) {pr_notice_once ("can-isotp: %s: can_send_ret %pe\n" , __func__,ERR_PTR (err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size; err_out_drop:/* claimed but nothing sent yet - no timer to cancel */ spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);return err; }
@@ -1259,22 +1397,26 @@ static int isotp_release(struct socket *sock) net =sock_net (sk);/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its * claim can't race the forced ISOTP_SHUTDOWN below; force it * unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {spin_unlock (&isotp_notifier_lock);schedule_timeout_uninterruptible (1 );
@@ -1385,11 +1527,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)* (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo * and RX watchdog timers bound how long this takes. Checked together * with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }/* ensure different CAN IDs when the rx_id is to be registered */
@@ -1419,11 +1562,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)if (isotp_register_rxid (so))can_rx_register (net, dev, rx_id,SINGLE_MASK (rx_id), isotp_rcv, sk,"isotp" , sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register (net, dev, tx_id,SINGLE_MASK (tx_id), isotp_rcv_echo, sk,"isotpe" , sk);
@@ -1762,11 +1905,11 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *w__poll_t mask =datagram_poll (file, sock, wait);poll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask; }
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH 2/2 can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
In reply to Oliver Hartkoppcommit 050f010f920da17c1044a4f174766ad553e770b6 upstream.
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
--- net/can/isotp.c | 317 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 230 insertions(+), 87 deletions(-)
net/can/isotp.c +230 -87
@@ -112,10 +112,19 @@ MODULE_ALIAS("can-proto-6");#define ISOTP_FC_OVFLW 2/* overflow */ #define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,ISOTP_WAIT_FC ,ISOTP_WAIT_DATA ,
@@ -148,21 +157,81 @@ struct isotp_sock {struct can_isotp_ll_options ll;u32 frame_txtime;u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;spinlock_t rx_lock;/* protect single thread state machine */ };static LIST_HEAD (isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk; }
@@ -181,20 +250,20 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) { struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */ sk->sk_err =ETIMEDOUT ;if (!sock_flag (sk,SOCK_DEAD )) sk->sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ; }
@@ -347,41 +416,42 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,static void isotp_send_cframe (struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ;if ((cf->len < ae +FC_CONTENT_SZ ) || ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, ae +FC_CONTENT_SZ , so->opt.rxpad_content))) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;if (!sock_flag (sk,SOCK_DEAD )) sk->sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get communication parameters only from the first FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC ) { +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];/* fix wrong STmin values according spec */ if (so->txfc.stmin > 0x7F &&
@@ -400,17 +470,17 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->txfc.stmin * 1000000); else so->tx_gap = ktime_add_ns(so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );isotp_send_cframe (so);break ;
@@ -421,18 +491,23 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) HRTIMER_MODE_REL_SOFT);break ;case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) - sk->sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) + sk->sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ; }
@@ -441,11 +516,11 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ; if ((so->opt.flags & ISOTP_CHECK_PADDING) &&
@@ -475,11 +550,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int i;int off;int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);/* the first frame has to use the entire frame up to LL_DL length */
@@ -519,11 +594,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)for (i = ae + ff_pci_sz; i < so->rx.ll_dl; i++) so->rx.buf[so->rx.idx++] = cf->data[i];/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )return 0 ;
@@ -537,11 +612,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */ if (so->opt.flags &CAN_ISOTP_FORCE_RXSTMIN ) { if (ktime_to_ns(ktime_sub(skb->tstamp, so->lastrxcf_tstamp)) <
@@ -552,15 +627,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, }hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */ if (cf->len > so->rx.ll_dl)return 1 ;
@@ -577,11 +650,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, sk->sk_err =EILSEQ ;if (!sock_flag (sk,SOCK_DEAD )) sk->sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++; so->rx.sn %=16 ;
@@ -591,11 +664,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,break ; }if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;
@@ -662,12 +735,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data)*/ spin_lock (&so->rx_lock);if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }switch (n_pci_type) {case N_PCI_FC :
@@ -757,10 +832,11 @@ static void isotp_send_cframe(struct isotp_sock *so)struct sk_buff *skb;struct net_device *dev;struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)return ;
@@ -771,10 +847,13 @@ static void isotp_send_cframe(struct isotp_sock *so) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */ isotp_fill_dataframe (cf, so, ae,0 );
@@ -787,16 +866,19 @@ static void isotp_send_cframe(struct isotp_sock *so) cf->flags = so->ll.tx_flags; skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );if (can_send_ret) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
@@ -844,11 +926,10 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,static void isotp_rcv_echo (struct sk_buff *skb,void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)return ;
@@ -856,36 +937,39 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)* (no isotp_rcv() caller here), so take it ourselves */ spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock; }
@@ -903,37 +987,41 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) out_unlock:spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )return HRTIMER_NORESTART ;/* only claim the timeout if the state is still unchanged */ if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) - sk->sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) + sk->sk_error_report (sk);wake_up_interruptible (&so->wait);return HRTIMER_NORESTART ; }
@@ -964,11 +1052,11 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)/* start echo timeout handling and cover below protocol error */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ; }
@@ -982,14 +1070,16 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ;int wait_tx_done = (so->opt.flags &CAN_ISOTP_WAIT_TX_DONE ) ?1 :0 ;s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim * with the RX path and with sendmsg()'s own error paths below, so * none of them can ever see a transfer mid-claim
@@ -1002,33 +1092,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* we do not support multiple buffers - for now */ if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ; goto err_out_drop; }
@@ -1066,19 +1160,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {/* The message size generally fits into a SingleFrame - good. *
@@ -1102,11 +1200,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[SF_PCI_SZ4 + ae] = size; else cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */ isotp_create_fframe (cf, so, ae);
@@ -1119,37 +1217,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* disable wait for FCs due to activated block size */ so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;/* start timeout for FC */ hrtimer_sec =ISOTP_FC_TIMEOUT ; tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */ spin_unlock_bh (&so->rx_lock);kfree_skb (skb);dev_put (dev);wake_up_interruptible (&so->wait);return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);/* send the first or only CAN frame */
@@ -1162,42 +1260,82 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (err) {pr_notice_once ("can-isotp: %s: can_send_ret %pe\n" , __func__,ERR_PTR (err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size; err_out_drop:/* claimed but nothing sent yet - no timer to cancel */ spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);return err; }
@@ -1261,22 +1399,26 @@ static int isotp_release(struct socket *sock) net =sock_net (sk);/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its * claim can't race the forced ISOTP_SHUTDOWN below; force it * unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {spin_unlock (&isotp_notifier_lock);schedule_timeout_uninterruptible (1 );
@@ -1387,11 +1529,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)* (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo * and RX watchdog timers bound how long this takes. Checked together * with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }/* ensure different CAN IDs when the rx_id is to be registered */
@@ -1421,11 +1564,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)if (isotp_register_rxid (so))can_rx_register (net, dev, rx_id,SINGLE_MASK (rx_id), isotp_rcv, sk,"isotp" , sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register (net, dev, tx_id,SINGLE_MASK (tx_id), isotp_rcv_echo, sk,"isotpe" , sk);
@@ -1764,11 +1907,11 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *w__poll_t mask =datagram_poll (file, sock, wait);poll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask; }
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH 2/2 can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
In reply to Oliver Hartkoppcommit 050f010f920da17c1044a4f174766ad553e770b6 upstream.
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
--- net/can/isotp.c | 317 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 230 insertions(+), 87 deletions(-)
net/can/isotp.c +230 -87
@@ -112,10 +112,19 @@ MODULE_ALIAS("can-proto-6");#define ISOTP_FC_OVFLW 2/* overflow */ #define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,ISOTP_WAIT_FC ,ISOTP_WAIT_DATA ,
@@ -148,21 +157,81 @@ struct isotp_sock {struct can_isotp_ll_options ll;u32 frame_txtime;u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;spinlock_t rx_lock;/* protect single thread state machine */ };static LIST_HEAD (isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk; }
@@ -181,20 +250,20 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) { struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */ sk->sk_err =ETIMEDOUT ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ; }
@@ -347,41 +416,42 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,static void isotp_send_cframe (struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ;if ((cf->len < ae +FC_CONTENT_SZ ) || ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, ae +FC_CONTENT_SZ , so->opt.rxpad_content))) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get communication parameters only from the first FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC ) { +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];/* fix wrong STmin values according spec */ if (so->txfc.stmin > 0x7F &&
@@ -400,17 +470,17 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->txfc.stmin * 1000000); else so->tx_gap = ktime_add_ns(so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );isotp_send_cframe (so);break ;
@@ -421,18 +491,23 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) HRTIMER_MODE_REL_SOFT);break ;case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ; }
@@ -441,11 +516,11 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ; if ((so->opt.flags & ISOTP_CHECK_PADDING) &&
@@ -475,11 +550,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int i;int off;int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);/* the first frame has to use the entire frame up to LL_DL length */
@@ -519,11 +594,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)for (i = ae + ff_pci_sz; i < so->rx.ll_dl; i++) so->rx.buf[so->rx.idx++] = cf->data[i];/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )return 0 ;
@@ -537,11 +612,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */ if (so->opt.flags &CAN_ISOTP_FORCE_RXSTMIN ) { if (ktime_to_ns(ktime_sub(skb->tstamp, so->lastrxcf_tstamp)) <
@@ -552,15 +627,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, }hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */ if (cf->len > so->rx.ll_dl)return 1 ;
@@ -577,11 +650,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, sk->sk_err =EILSEQ ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++; so->rx.sn %=16 ;
@@ -591,11 +664,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,break ; }if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;
@@ -662,12 +735,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data)*/ spin_lock (&so->rx_lock);if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }switch (n_pci_type) {case N_PCI_FC :
@@ -757,10 +832,11 @@ static void isotp_send_cframe(struct isotp_sock *so)struct sk_buff *skb;struct net_device *dev;struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)return ;
@@ -771,10 +847,13 @@ static void isotp_send_cframe(struct isotp_sock *so) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */ isotp_fill_dataframe (cf, so, ae,0 );
@@ -787,16 +866,19 @@ static void isotp_send_cframe(struct isotp_sock *so) cf->flags = so->ll.tx_flags; skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );if (can_send_ret) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
@@ -844,11 +926,10 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,static void isotp_rcv_echo (struct sk_buff *skb,void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)return ;
@@ -856,36 +937,39 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)* (no isotp_rcv() caller here), so take it ourselves */ spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock; }
@@ -903,37 +987,41 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) out_unlock:spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )return HRTIMER_NORESTART ;/* only claim the timeout if the state is still unchanged */ if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk);wake_up_interruptible (&so->wait);return HRTIMER_NORESTART ; }
@@ -964,11 +1052,11 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)/* start echo timeout handling and cover below protocol error */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ; }
@@ -982,14 +1070,16 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ;int wait_tx_done = (so->opt.flags &CAN_ISOTP_WAIT_TX_DONE ) ?1 :0 ;s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim * with the RX path and with sendmsg()'s own error paths below, so * none of them can ever see a transfer mid-claim
@@ -1002,33 +1092,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* we do not support multiple buffers - for now */ if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ; goto err_out_drop; }
@@ -1066,19 +1160,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {/* The message size generally fits into a SingleFrame - good. *
@@ -1102,11 +1200,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[SF_PCI_SZ4 + ae] = size; else cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */ isotp_create_fframe (cf, so, ae);
@@ -1119,37 +1217,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* disable wait for FCs due to activated block size */ so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;/* start timeout for FC */ hrtimer_sec =ISOTP_FC_TIMEOUT ; tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */ spin_unlock_bh (&so->rx_lock);kfree_skb (skb);dev_put (dev);wake_up_interruptible (&so->wait);return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);/* send the first or only CAN frame */
@@ -1162,42 +1260,82 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (err) {pr_notice_once ("can-isotp: %s: can_send_ret %pe\n" , __func__,ERR_PTR (err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size; err_out_drop:/* claimed but nothing sent yet - no timer to cancel */ spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);return err; }
@@ -1261,22 +1399,26 @@ static int isotp_release(struct socket *sock) net =sock_net (sk);/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its * claim can't race the forced ISOTP_SHUTDOWN below; force it * unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {spin_unlock (&isotp_notifier_lock);schedule_timeout_uninterruptible (1 );
@@ -1387,11 +1529,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)* (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo * and RX watchdog timers bound how long this takes. Checked together * with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }/* ensure different CAN IDs when the rx_id is to be registered */
@@ -1421,11 +1564,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)if (isotp_register_rxid (so))can_rx_register (net, dev, rx_id,SINGLE_MASK (rx_id), isotp_rcv, sk,"isotp" , sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register (net, dev, tx_id,SINGLE_MASK (tx_id), isotp_rcv_echo, sk,"isotpe" , sk);
@@ -1764,11 +1907,11 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *w__poll_t mask =datagram_poll (file, sock, wait);poll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask; }
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH v2 1/2 can: use skb hash instead of private variable in headroom
In reply to [email protected]commit d4fb6514ff8ed6912a71294e6b66a5d59ee88007 upstream.
The can_skb_priv::skbcnt variable is used to identify CAN skbs in the RX path analogue to the skb->hash.
As the skb hash is not filled in CAN skbs move the private skbcnt value to skb->hash and set skb->sw_hash accordingly. The skb->hash is a value used for RPS to identify skbs. Use it as intended.
--- drivers/net/can/dev/skb.c | 2 -- include/linux/can/core.h | 1 + include/linux/can/skb.h | 2 -- net/can/af_can.c | 14 +++++++++++--- net/can/bcm.c | 2 -- net/can/isotp.c | 3 --- net/can/j1939/socket.c | 1 - net/can/j1939/transport.c | 2 -- net/can/raw.c | 7 +++---- 9 files changed, 15 insertions(+), 19 deletions(-)
drivers/net/can/dev/skb.c +0 -2
@@ -200,11 +200,10 @@ static void init_can_skb_reserve(struct sk_buff *skb)skb_reset_mac_header (skb);skb_reset_network_header (skb);skb_reset_transport_header (skb);can_skb_reserve (skb); -can_skb_prv (skb)->skbcnt =0 ; }struct sk_buff *alloc_can_skb (struct net_device *dev,struct can_frame **cf) {struct sk_buff *skb;
@@ -310,11 +309,10 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)/* af_packet does not apply CAN skb specific settings */ if (skb->ip_summed ==CHECKSUM_NONE ) {/* init headroom */ can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->ip_summed =CHECKSUM_UNNECESSARY ;/* perform proper loopback on capable devices */ if (dev->flags &IFF_ECHO )
include/linux/can/core.h +1 -0
@@ -56,8 +56,9 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), void *data);extern int can_send (struct sk_buff *skb,int loop); +void can_set_skb_uid (struct sk_buff *skb);void can_sock_destruct (struct sock *sk);#endif /* !_CAN_CORE_H */
include/linux/can/skb.h +0 -2
@@ -47,17 +47,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);*/ /** * struct can_skb_priv - private additional data inside CAN sk_buffs * @ifindex: ifindex of the first interface the CAN frame appeared on -* @skbcnt: atomic counter to have an unique id together with skb pointer * @frame_len: length of CAN frame in data link layer * @cf: align to the following CAN frame at skb->data */ struct can_skb_priv {int ifindex; -int skbcnt;unsigned int frame_len;struct can_frame cf[]; };static inline struct can_skb_priv *can_skb_prv (struct sk_buff *skb)
net/can/af_can.c +11 -3
@@ -639,10 +639,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf }return matches; } +void can_set_skb_uid (struct sk_buff *skb) +{ +/* create non-zero unique skb identifier together with *skb */ +while (!(skb->hash)) + skb->hash =atomic_inc_return (&skbcounter); + + skb->sw_hash =1 ; +} +EXPORT_SYMBOL (can_set_skb_uid); +static void can_receive (struct sk_buff *skb,struct net_device *dev) {struct can_dev_rcv_lists *dev_rcv_lists;struct net *net =dev_net (dev);struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
@@ -650,13 +660,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)/* update statistics */ atomic_long_inc (&pkg_stats->rx_frames);atomic_long_inc (&pkg_stats->rx_frames_delta); -/* create non-zero unique skb identifier together with *skb */ -while (!(can_skb_prv (skb)->skbcnt)) -can_skb_prv (skb)->skbcnt =atomic_inc_return (&skbcounter); +can_set_skb_uid (skb);rcu_read_lock ();/* deliver the packet to sockets listening on all devices */ matches =can_rcv_filter (net->can.rx_alldev_list, skb);
net/can/bcm.c +0 -2
@@ -348,11 +348,10 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)if (!skb) goto out;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_put_data (skb, cf, op->cfsiz);/* send with loopback */ skb->dev = dev;
@@ -1619,11 +1618,10 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,kfree_skb (skb);return -ENODEV ; }can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skb->dev = dev;can_skb_set_owner (skb, sk); err =can_send (skb,1 );/* send with loopback */ dev_put (dev);
net/can/isotp.c +0 -3
@@ -231,11 +231,10 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)return 1 ; }can_skb_reserve (nskb);can_skb_prv (nskb)->ifindex = dev->ifindex; -can_skb_prv (nskb)->skbcnt =0 ; nskb->dev = dev;can_skb_set_owner (nskb, sk); ncf = (struct canfd_frame *)nskb->data;skb_put_zero (nskb, so->ll.mtu);
@@ -798,11 +797,10 @@ static void isotp_send_cframe(struct isotp_sock *so)return ; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */
@@ -1104,11 +1102,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto err_out_drop; }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;
net/can/j1939/socket.c +0 -1
@@ -895,11 +895,10 @@ static struct sk_buff *j1939_sk_alloc_skb(struct net_device *ndev,if (!skb) goto failure;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;skb_reserve (skb, offsetof(struct can_frame , data)); ret =memcpy_from_msg (skb_put (skb, size), msg, size);if (ret <0 ) goto free_skb;
net/can/j1939/transport.c +0 -2
@@ -610,11 +610,10 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,return ERR_PTR (-ENOMEM ); skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* reserve CAN header */ skb_reserve (skb, offsetof(struct can_frame , data));/* skb->cb must be large enough to hold a j1939_sk_buff_cb structure */ BUILD_BUG_ON (sizeof (skb->cb) <sizeof (*re_skcb));
@@ -1545,11 +1544,10 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,return NULL ; skb->dev = priv->ndev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = priv->ndev->ifindex; -can_skb_prv (skb)->skbcnt =0 ; skcb =j1939_skb_to_cb (skb);memcpy (skcb, rel_skcb,sizeof (*skcb)); session =j1939_session_new (priv, skb, size);if (!session) {
net/can/raw.c +3 -4
@@ -74,11 +74,11 @@ MODULE_ALIAS("can-proto-1");* storing the single filter in dfilter, to avoid using dynamic memory. */ struct uniqframe {const struct sk_buff *skb; -int skbcnt; +u32 hash;unsigned int join_rx_count; };struct raw_sock {struct sock sk;
@@ -162,21 +162,21 @@ static void raw_rcv(struct sk_buff *oskb, void *data) } }/* eliminate multiple filter matches for the same skb */ if (this_cpu_ptr (ro->uniq)->skb == oskb && -this_cpu_ptr (ro->uniq)->skbcnt ==can_skb_prv (oskb)->skbcnt) { +this_cpu_ptr (ro->uniq)->hash == oskb->hash) {if (!ro->join_filters)return ;this_cpu_inc (ro->uniq->join_rx_count);/* drop frame until all enabled filters matched */ if (this_cpu_ptr (ro->uniq)->join_rx_count < ro->count)return ; }else {this_cpu_ptr (ro->uniq)->skb = oskb; -this_cpu_ptr (ro->uniq)->skbcnt =can_skb_prv (oskb)->skbcnt; +this_cpu_ptr (ro->uniq)->hash = oskb->hash;this_cpu_ptr (ro->uniq)->join_rx_count =1 ;/* drop first frame to check all enabled filters? */ if (ro->join_filters && ro->count >1 )return ; }
@@ -952,11 +952,10 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (!skb) goto put_dev;can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; -can_skb_prv (skb)->skbcnt =0 ;/* fill the skb before testing for valid CAN frames */ err =memcpy_from_msg (skb_put (skb, size), msg, size);if (err <0 ) goto free_skb;
Signature · 1 line
2.53.0
Oliver Hartkopp
maintainerPATCH v2 2/2 can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
commit 050f010f920da17c1044a4f174766ad553e770b6 upstream.
This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize TX state transitions under so->rx_lock") which addresses following sashiko-bot findings:
- isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't re-arm echotimer after the claim
- isotp_release(): wake so->wait after forcing ISOTP_SHUTDOWN so a sleeping sendmsg() claim isn't stranded
- isotp_sendmsg(): have both wait_event_interruptible() calls in isotp_sendmsg() also wake on ISOTP_SHUTDOWN and do not return claim to IDLE to avoid corrupting a concurrent isotp_release() process.
- isotp_sendmsg(): handle potential claim of a new transfer when the wait_event_interruptible() call returns in CAN_ISOTP_WAIT_TX_DONE mode. Don't touch timers and states of the new transfer if a new thread incremented so->tx_gen before getting the lock at err_event_drop.
- isotp_sendmsg(): handle a stuck can_send() and omit timer and state changes if a new transfer was claimed. wait_tx_done() returns the error recorded in so->tx_result[], tagged with the caller's own generation.
- isotp_tx_timeout(): on a claimed timeout, record the ECOMM error for the timed-out transfer's own generation in so->tx_result[]; sk->sk_err is raised unconditionally, same as every other error path here.
- isotp_tx_gen_done()/isotp_tx_timeout(): always read tx.state (acquire) before tx_gen - the reverse order let a weakly ordered CPU pair a fresh tx.state with a stale tx_gen/tx_result slot.
- isotp_sendmsg(): wait_tx_done: drain sk_err via sock_error() once we have read the result from so->tx_result[], so an already-reported error doesn't stay latched for a later poll()/SO_ERROR.
Also align the remaining lock-free so->tx.state/rx.state/cfecho accesses and use skb->hash as unique loopback echo frame indicator.
--- net/can/isotp.c | 317 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 230 insertions(+), 87 deletions(-)
net/can/isotp.c +230 -87
@@ -124,10 +124,19 @@ MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "#define ISOTP_FC_OVFLW 2/* overflow */ #define ISOTP_FC_TIMEOUT 1/* 1 sec */ #define ISOTP_ECHO_TIMEOUT 2/* 2 secs */ +/* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed value +* (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation slot, so it +* can be handled with a single READ_ONCE()/WRITE_ONCE() access. +*/ +#define ISOTP_TX_RESULT_SLOTS 4 +#define ISOTP_TX_RESULT_GEN_BITS 24 +#define ISOTP_TX_RESULT_GEN_MASK ((1U << ISOTP_TX_RESULT_GEN_BITS) - 1) +#define ISOTP_TX_RESULT_ERR_MASK 0xFF +enum {ISOTP_IDLE =0 ,ISOTP_WAIT_FIRST_FC ,ISOTP_WAIT_FC ,ISOTP_WAIT_DATA ,
@@ -163,21 +172,81 @@ struct isotp_sock {struct can_isotp_ll_options ll;u32 frame_txtime;u32 force_tx_stmin;u32 force_rx_stmin;u32 cfecho;/* consecutive frame echo tag */ -u32 tx_gen;/* generation, bumped per new tx transfer */ +u32 tx_gen;/* transfer generation, increased per new tx transfer */ +u32 tx_result[ISOTP_TX_RESULT_SLOTS ];/* per-generation result slots */ struct tpcon rx, tx;struct list_head notifier;wait_queue_head_t wait;spinlock_t rx_lock;/* protect single thread state machine */ };static LIST_HEAD (isotp_notifier_list);static DEFINE_SPINLOCK (isotp_notifier_lock);static struct isotp_sock *isotp_busy_notifier; +/* increase (24 bit) tx generation value */ +static u32 isotp_inc_tx_gen (u32 gen) +{ +return (gen +1 ) &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* store 8 bit error and 24 bit tx generation values in packed u32 element */ +static u32 isotp_pack_tx_result (u32 gen,int err) +{ +return gen | ((u32 )err <<ISOTP_TX_RESULT_GEN_BITS ); +} + +/* get the 24 bit tx generation value from the tx result */ +static u32 isotp_get_tx_gen (u32 gen_err) +{ +return gen_err &ISOTP_TX_RESULT_GEN_MASK ; +} + +/* get the 8 bit error value from the tx result */ +static u32 isotp_get_tx_err (u32 gen_err) +{ +return (gen_err >>ISOTP_TX_RESULT_GEN_BITS ) &ISOTP_TX_RESULT_ERR_MASK ; +} + +/* store transfer result in per-generation%4 so->tx_result[] slot */ +static void isotp_set_tx_result (struct isotp_sock *so,u32 gen,int err) +{ +WRITE_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ], +isotp_pack_tx_result (gen, err)); +} + +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */ +static int isotp_get_tx_result (struct isotp_sock *so,u32 gen) +{ +u32 result =READ_ONCE (so->tx_result[gen %ISOTP_TX_RESULT_SLOTS ]); + +if (isotp_get_tx_gen (result) != gen) { +pr_notice_once ("can-isotp: tx_result[] slot reused before read\n" ); + +/* report failure rather than risk a false success */ +return -ECOMM ; + } + +return -(isotp_get_tx_err (result)); +} + +/* true if done, shut down or superseded ('gen' is no longer the active +* transfer). Reads tx.state first (acquire) so tx_gen/tx_result reads +* below see at least what that state write published (common sequence). +*/ +static bool isotp_tx_gen_done (struct isotp_sock *so,u32 gen) +{ +/* read tx.state first for the common sequence */ +u32 state =smp_load_acquire (&so->tx.state); + +return state ==ISOTP_IDLE || state ==ISOTP_SHUTDOWN || +READ_ONCE (so->tx_gen) != gen; +} +static inline struct isotp_sock *isotp_sk (const struct sock *sk) {return (struct isotp_sock *)sk; }
@@ -196,20 +265,20 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) { struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, rxtimer);struct sock *sk = &so->sk; -if (so->rx.state ==ISOTP_WAIT_DATA ) { +if (READ_ONCE (so->rx.state) ==ISOTP_WAIT_DATA ) {/* we did not get new data frames in time */ /* report 'connection timed out' */ sk->sk_err =ETIMEDOUT ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); }return HRTIMER_NORESTART ; }
@@ -363,41 +432,42 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,static void isotp_send_cframe (struct isotp_sock *so);static int isotp_rcv_fc (struct isotp_sock *so,struct canfd_frame *cf,int ae) {struct sock *sk = &so->sk; +int tx_err =EBADMSG ;/* default for unknown FC status */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 0 ;hrtimer_cancel (&so->txtimer);/* isotp_tx_timeout() may have given up on this job while -* hrtimer_cancel() above waited for it to finish; so->rx_lock -* (held by our caller isotp_rcv()) rules out a concurrent claim, -* so a plain recheck is enough here. +* hrtimer_cancel() above waited for it to finish => recheck */ -if (so->tx.state !=ISOTP_WAIT_FC && - so->tx.state !=ISOTP_WAIT_FIRST_FC ) +if (READ_ONCE (so->tx.state) !=ISOTP_WAIT_FC && +READ_ONCE (so->tx.state) !=ISOTP_WAIT_FIRST_FC )return 1 ;if ((cf->len < ae +FC_CONTENT_SZ ) || ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, ae +FC_CONTENT_SZ , so->opt.rxpad_content))) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk); - so->tx.state =ISOTP_IDLE ; +isotp_set_tx_result (so, so->tx_gen,EBADMSG ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait);return 1 ; }/* get static/dynamic communication params from first/every FC frame */ -if (so->tx.state ==ISOTP_WAIT_FIRST_FC || +if (READ_ONCE (so->tx.state) ==ISOTP_WAIT_FIRST_FC || so->opt.flags &CAN_ISOTP_DYN_FC_PARMS ) { so->txfc.bs = cf->data[ae +1 ]; so->txfc.stmin = cf->data[ae +2 ];/* fix wrong STmin values according spec */
@@ -417,17 +487,17 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) so->txfc.stmin * 1000000); else so->tx_gap = ktime_add_ns(so->tx_gap, (so->txfc.stmin -0xF0 ) *100000 ); - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC ); }switch (cf->data[ae] &0x0F ) {case ISOTP_FC_CTS : so->tx.bs =0 ; - so->tx.state =ISOTP_SENDING ; +WRITE_ONCE (so->tx.state,ISOTP_SENDING );/* send CF frame and enable echo timeout handling */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );isotp_send_cframe (so);break ;
@@ -438,18 +508,23 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) HRTIMER_MODE_REL_SOFT);break ;case ISOTP_FC_OVFLW :/* overflow on receiver side - report 'message too long' */ - sk->sk_err =EMSGSIZE ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); + tx_err =EMSGSIZE ; fallthrough;default : -/* stop this tx job */ - so->tx.state =ISOTP_IDLE ; +/* reserved/unknown flow status (tx_err defaults to EBADMSG) */ + + sk->sk_err = tx_err; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk); + +isotp_set_tx_result (so, so->tx_gen, tx_err); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); }return 0 ; }
@@ -458,11 +533,11 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if (!len || len > cf->len - pcilen)return 1 ; if ((so->opt.flags & ISOTP_CHECK_PADDING) &&
@@ -492,11 +567,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)int i;int off;int ff_pci_sz;hrtimer_cancel (&so->rxtimer); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );/* get the used sender LL_DL from the (first) CAN frame data length */ so->rx.ll_dl =padlen (cf->len);/* the first frame has to use the entire frame up to LL_DL length */
@@ -546,11 +621,11 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)for (i = ae + ff_pci_sz; i < so->rx.ll_dl; i++) so->rx.buf[so->rx.idx++] = cf->data[i];/* initial setup for this pdu reception */ so->rx.sn =1 ; - so->rx.state =ISOTP_WAIT_DATA ; +WRITE_ONCE (so->rx.state,ISOTP_WAIT_DATA );/* no creation of flow control frames */ if (so->opt.flags &CAN_ISOTP_LISTEN_MODE )return 0 ;
@@ -564,11 +639,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, {struct isotp_sock *so =isotp_sk (sk);struct sk_buff *nskb;int i; -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 0 ;/* drop if timestamp gap is less than force_rx_stmin nano secs */ if (so->opt.flags &CAN_ISOTP_FORCE_RXSTMIN ) { if (ktime_to_ns(ktime_sub(skb->tstamp, so->lastrxcf_tstamp)) <
@@ -579,15 +654,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, }hrtimer_cancel (&so->rxtimer);/* isotp_rx_timer_handler() may have raced us for so->rx.state -* while hrtimer_cancel() above waited for it to finish, already -* reporting ETIMEDOUT and resetting the reception; don't process -* this CF into a reassembly that has already been given up on. +* while hrtimer_cancel() above waited for it to finish => recheck */ -if (so->rx.state !=ISOTP_WAIT_DATA ) +if (READ_ONCE (so->rx.state) !=ISOTP_WAIT_DATA )return 1 ;/* CFs are never longer than the FF */ if (cf->len > so->rx.ll_dl)return 1 ;
@@ -604,11 +677,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, sk->sk_err =EILSEQ ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);/* reset rx state */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );return 1 ; } so->rx.sn++; so->rx.sn %=16 ;
@@ -618,11 +691,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,break ; }if (so->rx.idx >= so->rx.len) {/* we are done */ - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE );if ((so->opt.flags &ISOTP_CHECK_PADDING ) &&check_pad (so, cf, i +1 , so->opt.rxpad_content)) {/* malformed PDU - report 'not a data message' */ sk->sk_err =EBADMSG ;
@@ -689,12 +762,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data)*/ spin_lock (&so->rx_lock);if (so->opt.flags &CAN_ISOTP_HALF_DUPLEX ) {/* check rx/tx path half duplex expectations */ -if ((so->tx.state !=ISOTP_IDLE && n_pci_type !=N_PCI_FC ) || - (so->rx.state !=ISOTP_IDLE && n_pci_type ==N_PCI_FC )) +if ((READ_ONCE (so->tx.state) !=ISOTP_IDLE && + n_pci_type !=N_PCI_FC ) || + (READ_ONCE (so->rx.state) !=ISOTP_IDLE && + n_pci_type ==N_PCI_FC )) goto out_unlock; }switch (n_pci_type) {case N_PCI_FC :
@@ -784,10 +859,11 @@ static void isotp_send_cframe(struct isotp_sock *so)struct sk_buff *skb;struct net_device *dev;struct canfd_frame *cf;int can_send_ret;int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ; +u32 old_cfecho; dev =dev_get_by_index (sock_net (sk), so->ifindex);if (!dev)return ;
@@ -798,10 +874,13 @@ static void isotp_send_cframe(struct isotp_sock *so) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* create consecutive frame */ isotp_fill_dataframe (cf, so, ae,0 );
@@ -814,16 +893,19 @@ static void isotp_send_cframe(struct isotp_sock *so) cf->flags = so->ll.tx_flags; skb->dev = dev;can_skb_set_owner (skb, sk); -/* cfecho should have been zero'ed by init/isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , so->cfecho); +/* zero'ed by init/isotp_rcv_echo(); reached lock-free via +* isotp_txfr_timer_handler() too, so use READ_ONCE()/WRITE_ONCE() +*/ + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: cfecho is %08X != 0\n" , old_cfecho);/* set consecutive frame echo tag */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash);/* send frame with local echo enabled */ can_send_ret =can_send (skb,1 );if (can_send_ret) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
@@ -871,11 +953,10 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,static void isotp_rcv_echo (struct sk_buff *skb,void *data) {struct sock *sk = (struct sock *)data;struct isotp_sock *so =isotp_sk (sk); -struct canfd_frame *cf = (struct canfd_frame *)skb->data;/* only handle my own local echo CF/SF skb's (no FF!) */ if (skb->sk != sk)return ;
@@ -883,36 +964,39 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)* (no isotp_rcv() caller here), so take it ourselves */ spin_lock (&so->rx_lock);/* so->cfecho may since belong to a new transfer; recheck under lock */ -if (so->cfecho != *(u32 *)cf->data) +if (READ_ONCE (so->cfecho) != skb->hash) goto out_unlock;/* cancel local echo timeout */ hrtimer_cancel (&so->echotimer);/* local echo skb with consecutive frame has been consumed */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* claiming a transfer also takes so->rx_lock, so a plain recheck * is enough: so->tx.state can't have flipped to ISOTP_SENDING for * a new claim while we're still in here */ -if (so->tx.state !=ISOTP_SENDING ) +if (READ_ONCE (so->tx.state) !=ISOTP_SENDING ) goto out_unlock;if (so->tx.idx >= so->tx.len) {/* we are done */ - so->tx.state =ISOTP_IDLE ; + +isotp_set_tx_result (so, so->tx_gen,0 ); +/* set to IDLE after publishing tx_result */ +smp_store_release (&so->tx.state,ISOTP_IDLE );wake_up_interruptible (&so->wait); goto out_unlock; }if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {/* stop and wait for FC with timeout */ - so->tx.state =ISOTP_WAIT_FC ; +WRITE_ONCE (so->tx.state,ISOTP_WAIT_FC );hrtimer_start (&so->txtimer,ktime_set (ISOTP_FC_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT ); goto out_unlock; }
@@ -930,37 +1014,41 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) out_unlock:spin_unlock (&so->rx_lock); } -/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get -* cancelled under so->rx_lock elsewhere, so this must stay lock-free to -* avoid deadlocking with that; uses so->tx_gen instead to avoid tainting -* a new transfer with an error from the one that just timed out. +/* isotp_tx_timeout: we did not get any flow control or echo frame in time +* +* Shared by so->txtimer's and so->echotimer's callbacks. Both timers get +* cancelled under so->rx_lock elsewhere, so this must stay lock-free. +* +* tx.state is acquired before tx_gen. Common sequence in isotp_tx_gen_done(). +* cmpxchg() only orders itself, not the two preceding loads. */ static enum hrtimer_restart isotp_tx_timeout (struct isotp_sock *so) {struct sock *sk = &so->sk; +/* read tx.state first for the common sequence */ +u32 old_state =smp_load_acquire (&so->tx.state);u32 gen =READ_ONCE (so->tx_gen); -u32 old_state =READ_ONCE (so->tx.state);/* don't handle timeouts in IDLE or SHUTDOWN state */ if (old_state ==ISOTP_IDLE || old_state ==ISOTP_SHUTDOWN )return HRTIMER_NORESTART ;/* only claim the timeout if the state is still unchanged */ if (cmpxchg (&so->tx.state, old_state,ISOTP_IDLE ) != old_state)return HRTIMER_NORESTART ; -/* we did not get any flow control or echo frame in time */ +/* detected timeout: report 'communication error on send' */ -if (READ_ONCE (so->tx_gen) == gen) { -/* report 'communication error on send' */ - sk->sk_err =ECOMM ; -if (!sock_flag (sk,SOCK_DEAD )) -sk_error_report (sk); - } +/* a stale read of this slot by a waiter still falls back to ECOMM */ +isotp_set_tx_result (so, gen,ECOMM ); + + sk->sk_err =ECOMM ; +if (!sock_flag (sk,SOCK_DEAD )) +sk_error_report (sk);wake_up_interruptible (&so->wait);return HRTIMER_NORESTART ; }
@@ -991,11 +1079,11 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)/* start echo timeout handling and cover below protocol error */ hrtimer_start (&so->echotimer,ktime_set (ISOTP_ECHO_TIMEOUT ,0 ),HRTIMER_MODE_REL_SOFT );/* cfecho should be consumed by isotp_rcv_echo() here */ -if (so->tx.state ==ISOTP_SENDING && !so->cfecho) +if (READ_ONCE (so->tx.state) ==ISOTP_SENDING && !READ_ONCE (so->cfecho))isotp_send_cframe (so);return HRTIMER_NORESTART ; }
@@ -1009,14 +1097,16 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)int ae = (so->opt.flags &CAN_ISOTP_EXTEND_ADDR ) ?1 :0 ;int wait_tx_done = (so->opt.flags &CAN_ISOTP_WAIT_TX_DONE ) ?1 :0 ;s64 hrtimer_sec =ISOTP_ECHO_TIMEOUT ;struct hrtimer *tx_hrt = &so->echotimer;u32 new_state =ISOTP_SENDING ; +u32 my_gen; +u32 old_cfecho;int off;int err; -if (!so->bound || so->tx.state ==ISOTP_SHUTDOWN ) +if (!so->bound ||READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* claim the socket under so->rx_lock: this serializes the claim * with the RX path and with sendmsg()'s own error paths below, so * none of them can ever see a transfer mid-claim
@@ -1029,33 +1119,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* we do not support multiple buffers - for now */ if (msg->msg_flags &MSG_DONTWAIT )return -EAGAIN ; -if (so->tx.state ==ISOTP_SHUTDOWN ) +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN )return -EADDRNOTAVAIL ;/* wait for complete transmission of current pdu */ err =wait_event_interruptible (so->wait, - so->tx.state ==ISOTP_IDLE ); +READ_ONCE (so->tx.state) ==ISOTP_IDLE || +READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN );if (err)return err; } -/* new transfer: bump so->tx_gen and drain the old one's timers, -* still under the so->rx_lock we just claimed the socket with -*/ -WRITE_ONCE (so->tx.state,ISOTP_SENDING ); -WRITE_ONCE (so->tx_gen,READ_ONCE (so->tx_gen) +1 ); +/* txfrtimer's callback re-arms echotimer lock-free: drain it first */ +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); -hrtimer_cancel (&so->txfrtimer); - so->cfecho =0 ; + +/* new transfer: increment so->tx_gen and set tx.state after barrier */ + my_gen =isotp_inc_tx_gen (READ_ONCE (so->tx_gen)); +isotp_set_tx_result (so, my_gen,ECOMM );/* prevent stale slot matching */ +WRITE_ONCE (so->tx_gen, my_gen); +smp_wmb ();/* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */ +WRITE_ONCE (so->tx.state,ISOTP_SENDING ); +WRITE_ONCE (so->cfecho,0 );spin_unlock_bh (&so->rx_lock);/* so->bound is only checked once above - a wakeup may have -* unbound/rebound the socket meanwhile, so re-validate it +* unbound/rebound the socket meanwhile => recheck */ if (!so->bound) { err = -EADDRNOTAVAIL ; goto err_out_drop; }
@@ -1103,19 +1197,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) }can_skb_reserve (skb);can_skb_prv (skb)->ifindex = dev->ifindex; +/* set uid in tx skb to identify CF echo frames */ +can_set_skb_uid (skb); + so->tx.len = size; so->tx.idx =0 ; cf = (struct canfd_frame *)skb->data;skb_put_zero (skb, so->ll.mtu);/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ -if (so->cfecho) -pr_notice_once ("can-isotp: uninit cfecho %08X\n" , so->cfecho); + old_cfecho =READ_ONCE (so->cfecho); +if (old_cfecho) +pr_notice_once ("can-isotp: uninit cfecho %08X\n" , old_cfecho);/* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl -SF_PCI_SZ4 - ae - off) {/* The message size generally fits into a SingleFrame - good. *
@@ -1139,11 +1237,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf->data[SF_PCI_SZ4 + ae] = size; else cf->data[ae] |= size;/* set CF echo tag for isotp_rcv_echo() (SF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* send first frame */ isotp_create_fframe (cf, so, ae);
@@ -1156,37 +1254,37 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)/* disable wait for FCs due to activated block size */ so->txfc.bs =0 ;/* set CF echo tag for isotp_rcv_echo() (CF-mode) */ - so->cfecho = *(u32 *)cf->data; +WRITE_ONCE (so->cfecho, skb->hash); }else {/* standard flow control check */ new_state =ISOTP_WAIT_FIRST_FC ;/* start timeout for FC */ hrtimer_sec =ISOTP_FC_TIMEOUT ; tx_hrt = &so->txtimer;/* no CF echo tag for isotp_rcv_echo() (FF-mode) */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 ); } }spin_lock_bh (&so->rx_lock); -if (so->tx.state ==ISOTP_SHUTDOWN ) { +if (READ_ONCE (so->tx.state) ==ISOTP_SHUTDOWN ) {/* isotp_release() has since taken over and already drained * our timers - don't send into a socket that's going away */ spin_unlock_bh (&so->rx_lock);kfree_skb (skb);dev_put (dev);wake_up_interruptible (&so->wait);return -EADDRNOTAVAIL ; }/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */ - so->tx.state = new_state; +WRITE_ONCE (so->tx.state, new_state);hrtimer_start (tx_hrt,ktime_set (hrtimer_sec,0 ),HRTIMER_MODE_REL_SOFT );spin_unlock_bh (&so->rx_lock);/* send the first or only CAN frame */
@@ -1199,42 +1297,82 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)if (err) {pr_notice_once ("can-isotp: %s: can_send_ret %pe\n" , __func__,ERR_PTR (err));spin_lock_bh (&so->rx_lock); + +/* new transfer already claimed by a concurrent completion, +* timeout or sendmsg() while we were stuck in can_send()? +*/ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and state of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +/* no transmission -> no timeout monitoring */ hrtimer_cancel (tx_hrt); goto err_out_drop_locked; }if (wait_tx_done) { -/* wait for complete transmission of current pdu */ - err =wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ); +/* wake up for: +* - concurrent sendmsg() claiming a new transfer +* - complete transmission of current PDU +* - shutdown state change in isotp_release() +* isotp_tx_gen_done() uses common tx.state/tx_gen read sequence +*/ + err =wait_event_interruptible (so->wait, +isotp_tx_gen_done (so, my_gen));if (err) goto err_event_drop; - err =sock_error (sk); -if (err) -return err; +/* still our claim, but isotp_release() force-shut it down */ +if (smp_load_acquire (&so->tx.state) ==ISOTP_SHUTDOWN && +READ_ONCE (so->tx_gen) == my_gen) { + err = -EADDRNOTAVAIL ; + goto err_event_drop; + } + +/* own completion, or tx_gen moved on - either way this is +* what isotp_get_tx_result() recorded for my_gen +*/ + err =isotp_get_tx_result (so, my_gen); + +/* drain to avoid stale error for a later poll()/SO_ERROR */ +sock_error (sk); + +return err ? err : size; }return size; err_out_drop:/* claimed but nothing sent yet - no timer to cancel */ spin_lock_bh (&so->rx_lock); goto err_out_drop_locked; err_event_drop: -/* interrupted waiting on our own transfer - drain its timers */ +/* interrupted or shut down while waiting on our own transfer */ spin_lock_bh (&so->rx_lock); + +/* new transfer already started by concurrent sendmsg()? */ +if (READ_ONCE (so->tx_gen) != my_gen) { +/* don't touch timers and states of the new transfer */ +spin_unlock_bh (&so->rx_lock); +return err; + } +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->echotimer); err_out_drop_locked:/* release the claim; so->rx_lock still held from above */ - so->cfecho =0 ; - so->tx.state =ISOTP_IDLE ; +WRITE_ONCE (so->cfecho,0 ); + +/* only claim to IDLE if isotp_release() has not taken over */ +if (READ_ONCE (so->tx.state) !=ISOTP_SHUTDOWN ) +WRITE_ONCE (so->tx.state,ISOTP_IDLE );spin_unlock_bh (&so->rx_lock);wake_up_interruptible (&so->wait);return err; }
@@ -1296,22 +1434,26 @@ static int isotp_release(struct socket *sock) net =sock_net (sk);/* best-effort: wait for a running pdu to finish, but don't block on * it forever - give up after the first signal */ -while (so->tx.state !=ISOTP_IDLE && -wait_event_interruptible (so->wait, so->tx.state ==ISOTP_IDLE ) ==0 ) +while (READ_ONCE (so->tx.state) !=ISOTP_IDLE && +wait_event_interruptible (so->wait, +READ_ONCE (so->tx.state) ==ISOTP_IDLE ) ==0 ) ;/* claim the socket under so->rx_lock like sendmsg() does, so its * claim can't race the forced ISOTP_SHUTDOWN below; force it * unconditionally, even when a signal cut the wait above short */ spin_lock_bh (&so->rx_lock); - so->tx.state =ISOTP_SHUTDOWN ; +WRITE_ONCE (so->tx.state,ISOTP_SHUTDOWN );spin_unlock_bh (&so->rx_lock); - so->rx.state =ISOTP_IDLE ; +WRITE_ONCE (so->rx.state,ISOTP_IDLE ); + +/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */ +wake_up_interruptible (&so->wait);spin_lock (&isotp_notifier_lock);while (isotp_busy_notifier == so) {spin_unlock (&isotp_notifier_lock);schedule_timeout_uninterruptible (1 );
@@ -1423,11 +1565,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)* (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo * and RX watchdog timers bound how long this takes. Checked together * with so->bound in the same lock_sock() section above, so there is * no window in which a concurrent isotp_notify() could be missed. */ -if (so->tx.state !=ISOTP_IDLE || so->rx.state !=ISOTP_IDLE ) { +if (READ_ONCE (so->tx.state) !=ISOTP_IDLE || +READ_ONCE (so->rx.state) !=ISOTP_IDLE ) { err = -EAGAIN ; goto out; }/* ensure different CAN IDs when the rx_id is to be registered */
@@ -1457,11 +1600,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)if (isotp_register_rxid (so))can_rx_register (net, dev, rx_id,SINGLE_MASK (rx_id), isotp_rcv, sk,"isotp" , sk);/* no consecutive frame echo skb in flight */ - so->cfecho =0 ; +WRITE_ONCE (so->cfecho,0 );/* register for echo skb's */ can_rx_register (net, dev, tx_id,SINGLE_MASK (tx_id), isotp_rcv_echo, sk,"isotpe" , sk);
@@ -1823,11 +1966,11 @@ static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *w__poll_t mask =datagram_poll (file, sock, wait);poll_wait (file, &so->wait, wait);/* Check for false positives due to TX state */ -if ((mask &EPOLLWRNORM ) && (so->tx.state !=ISOTP_IDLE )) +if ((mask &EPOLLWRNORM ) && (READ_ONCE (so->tx.state) !=ISOTP_IDLE )) mask &= ~(EPOLLOUT |EPOLLWRNORM );return mask; }
Signature · 1 line
2.53.0