| Message ID | 20221116165943.1776754-6-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hsr: HSR send/recv fixes | expand |
Hi Sebastian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Andrzej-Siewior/hsr-HSR-send-recv-fixes/20221117-010114
patch link: https://lore.kernel.org/r/20221116165943.1776754-6-bigeasy%40linutronix.de
patch subject: [PATCH net 5/5] hsr: Synchronize sequence number updates.
config: riscv-randconfig-r003-20221117
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 463da45892e2d2a262277b91b96f5f8c05dc25d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/d50e5c90d21a4d0291280f2c7dc5287d2597bf56
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sebastian-Andrzej-Siewior/hsr-HSR-send-recv-fixes/20221117-010114
git checkout d50e5c90d21a4d0291280f2c7dc5287d2597bf56
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/hsr/hsr_framereg.c:465:6: error: call to '__compiletime_assert_370' declared with 'error' attribute: BUILD_BUG failed
if (cmpxchg(&node->seq_out[port->type], old_seq, sequence_nr) != old_seq)
^
include/linux/atomic/atomic-instrumented.h:1916:2: note: expanded from macro 'cmpxchg'
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
^
arch/riscv/include/asm/cmpxchg.h:344:23: note: expanded from macro 'arch_cmpxchg'
(__typeof__(*(ptr))) __cmpxchg((ptr), \
^
arch/riscv/include/asm/cmpxchg.h:335:3: note: expanded from macro '__cmpxchg'
BUILD_BUG(); \
^
note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:345:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
include/linux/compiler_types.h:338:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:3:1: note: expanded from here
__compiletime_assert_370
^
1 error generated.
vim +465 net/hsr/hsr_framereg.c
444
445 /* 'skb' is a HSR Ethernet frame (with a HSR tag inserted), with a valid
446 * ethhdr->h_source address and skb->mac_header set.
447 *
448 * Return:
449 * 1 if frame can be shown to have been sent recently on this interface,
450 * 0 otherwise, or
451 * negative error code on error
452 */
453 int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
454 u16 sequence_nr)
455 {
456 u16 old_seq;
457 again:
458 old_seq = READ_ONCE(node->seq_out[port->type]);
459
460 if (seq_nr_before_or_eq(sequence_nr, old_seq) &&
461 time_is_after_jiffies(node->time_out[port->type] +
462 msecs_to_jiffies(HSR_ENTRY_FORGET_TIME)))
463 return 1;
464
> 465 if (cmpxchg(&node->seq_out[port->type], old_seq, sequence_nr) != old_seq)
466 goto again;
467
468 node->time_out[port->type] = jiffies;
469 return 0;
470 }
471
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c index 9b8eaebce2549..7a9d4d36f114d 100644 --- a/net/hsr/hsr_framereg.c +++ b/net/hsr/hsr_framereg.c @@ -453,13 +453,19 @@ void hsr_register_frame_in(struct hsr_node *node, struct hsr_port *port, int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node, u16 sequence_nr) { - if (seq_nr_before_or_eq(sequence_nr, node->seq_out[port->type]) && + u16 old_seq; +again: + old_seq = READ_ONCE(node->seq_out[port->type]); + + if (seq_nr_before_or_eq(sequence_nr, old_seq) && time_is_after_jiffies(node->time_out[port->type] + msecs_to_jiffies(HSR_ENTRY_FORGET_TIME))) return 1; + if (cmpxchg(&node->seq_out[port->type], old_seq, sequence_nr) != old_seq) + goto again; + node->time_out[port->type] = jiffies; - node->seq_out[port->type] = sequence_nr; return 0; }
hsr_register_frame_out() compares new sequence_nr vs the old one recorded in hsr_node::seq_out and if the new sequence_nr is higher then it will be written to hsr_node::seq_out as the new value. This operation isn't locked so it is possible that two frames with the same sequence number arrive (via the two slave devices) and are fed to hsr_register_frame_out() at the same time. Both will pass the check and update the sequence counter later to the same value. As a result the content of the same packet is fed into the stack twice. This was noticed by running ping and observing DUP being reported from time to time. Instead of using the hsr_priv::seqnr_lock for the whole receive path (as it is for sending in the master node) ensure that the id is only updated based on the expected old value. Use cmpxchg() to only update sequence number if it is the old value, that was also used for comparison. Fixes: f421436a591d3 ("net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)") Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- net/hsr/hsr_framereg.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)