[PATCH net 0/4] bonding: fix send_peer_notif overflow

[PATCH net 0/4] bonding: fix send_peer_notif overflow

[Hangbin Liu]

Bonding send_peer_notif was defined as u8. But the value is
num_peer_notif multiplied by peer_notif_delay, which is u8 * u32.
This would cause the send_peer_notif overflow.

Before the fix:
TEST: num_grat_arp (active-backup miimon num_grat_arp 10)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 20)           [ OK ]
4 garp packets sent on active slave eth1
TEST: num_grat_arp (active-backup miimon num_grat_arp 30)           [FAIL]
24 garp packets sent on active slave eth1
TEST: num_grat_arp (active-backup miimon num_grat_arp 50)           [FAIL]

After the fix:
TEST: num_grat_arp (active-backup miimon num_grat_arp 10)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 20)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 30)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 50)           [ OK ]

Hangbin Liu (4):
  bonding: fix send_peer_notif overflow
  Documentation: bonding: fix the doc of peer_notif_delay
  selftests: forwarding: lib: add netns support for tc rule handle stats
    get
  kselftest: bonding: add num_grat_arp test

 Documentation/networking/bonding.rst          |  7 ++-
 include/net/bonding.h                         |  2 +-
 .../drivers/net/bonding/bond_options.sh       | 50 +++++++++++++++++++
 .../drivers/net/bonding/bond_topo_3d1c.sh     |  2 +
 tools/testing/selftests/net/forwarding/lib.sh |  3 +-
 5 files changed, 58 insertions(+), 6 deletions(-)

-- 
2.38.1

[PATCH net 1/4] bonding: fix send_peer_notif overflow

[Hangbin Liu]

Bonding send_peer_notif was defined as u8. Since commit 07a4ddec3ce9
("bonding: add an option to specify a delay between peer notifications").
the bond->send_peer_notif will be num_peer_notif multiplied by
peer_notif_delay, which is u8 * u32. This would cause the send_peer_notif
overflow easily. e.g.

  ip link add bond0 type bond mode 1 miimon 100 num_grat_arp 30 peer_notify_delay 1000

To fix the overflow, we have to set the send_peer_notif large enough.

Fixes: 07a4ddec3ce9 ("bonding: add an option to specify a delay between peer notifications")
Reported-by: Liang Li <liali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/bonding.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/bonding.h b/include/net/bonding.h
index c3843239517d..f4d376420e4d 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -233,7 +233,7 @@ struct bonding {
 	 */
 	spinlock_t mode_lock;
 	spinlock_t stats_lock;
-	u8	 send_peer_notif;
+	u64	 send_peer_notif;
 	u8       igmp_retrans;
 #ifdef CONFIG_PROC_FS
 	struct   proc_dir_entry *proc_entry;
-- 
2.38.1

[PATCH net 2/4] Documentation: bonding: fix the doc of peer_notif_delay

[Hangbin Liu]

Bonding only supports setting peer_notif_delay with miimon set.

Fixes: 0307d589c4d6 ("bonding: add documentation for peer_notif_delay")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 Documentation/networking/bonding.rst | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index adc4bf4f3c50..6daeb18911fb 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -776,10 +776,9 @@ peer_notif_delay
 	Specify the delay, in milliseconds, between each peer
 	notification (gratuitous ARP and unsolicited IPv6 Neighbor
 	Advertisement) when they are issued after a failover event.
-	This delay should be a multiple of the link monitor interval
-	(arp_interval or miimon, whichever is active). The default
-	value is 0 which means to match the value of the link monitor
-	interval.
+	This delay should be a multiple of the MII link monitor interval
+	(miimon). The default value is 0 which means to match the value
+        of the MII link monitor interval.
 
 prio
 	Slave priority. A higher number means higher priority.
-- 
2.38.1

[PATCH net 3/4] selftests: forwarding: lib: add netns support for tc rule handle stats get

[Hangbin Liu]

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index d47499ba81c7..426bab05fe0a 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -773,8 +773,9 @@ tc_rule_handle_stats_get()
 	local id=$1; shift
 	local handle=$1; shift
 	local selector=${1:-.packets}; shift
+	local netns=${1:-""}; shift
 
-	tc -j -s filter show $id \
+	tc $netns -j -s filter show $id \
 	    | jq ".[] | select(.options.handle == $handle) | \
 		  .options.actions[0].stats$selector"
 }
-- 
2.38.1

[PATCH net 4/4] kselftest: bonding: add num_grat_arp test

[Hangbin Liu]

TEST: num_grat_arp (active-backup miimon num_grat_arp 10)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 20)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 30)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 50)           [ OK ]

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../drivers/net/bonding/bond_options.sh       | 50 +++++++++++++++++++
 .../drivers/net/bonding/bond_topo_3d1c.sh     |  2 +
 2 files changed, 52 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index db29a3146a86..607ba5c38977 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -6,6 +6,7 @@
 ALL_TESTS="
 	prio
 	arp_validate
+	num_grat_arp
 "
 
 REQUIRE_MZ=no
@@ -255,6 +256,55 @@ arp_validate()
 	arp_validate_ns "active-backup"
 }
 
+garp_test()
+{
+	local param="$1"
+	local active_slave exp_num real_num i
+	RET=0
+
+	# create bond
+	bond_reset "${param}"
+
+	bond_check_connection
+	[ $RET -ne 0 ] && log_test "num_grat_arp" "$retmsg"
+
+
+	# Add tc rules to count GARP number
+	for i in $(seq 0 2); do
+		tc -n ${g_ns} filter add dev s$i ingress protocol arp pref 1 handle 101 \
+			flower skip_hw arp_op request arp_sip ${s_ip4} arp_tip ${s_ip4} action pass
+	done
+
+	# Do failover
+	active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
+	ip -n ${s_ns} link set ${active_slave} down
+
+	exp_num=$(echo "${param}" | cut -f6 -d ' ')
+	sleep $((exp_num + 2))
+
+	active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
+
+	# check result
+	real_num=$(tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}")
+	if [ "${real_num}" -ne "${exp_num}" ]; then
+		echo "$real_num garp packets sent on active slave ${active_slave}"
+		RET=1
+	fi
+
+	for i in $(seq 0 2); do
+		tc -n ${g_ns} filter del dev s$i ingress
+	done
+}
+
+num_grat_arp()
+{
+	local val
+	for val in 10 20 30 50; do
+		garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 1000"
+		log_test "num_grat_arp" "active-backup miimon num_grat_arp $val"
+	done
+}
+
 trap cleanup EXIT
 
 setup_prepare
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_topo_3d1c.sh b/tools/testing/selftests/drivers/net/bonding/bond_topo_3d1c.sh
index 4045ca97fb22..69ab99a56043 100644
--- a/tools/testing/selftests/drivers/net/bonding/bond_topo_3d1c.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_topo_3d1c.sh
@@ -61,6 +61,8 @@ server_create()
 		ip -n ${g_ns} link set s${i} up
 		ip -n ${g_ns} link set s${i} master br0
 		ip -n ${s_ns} link set eth${i} master bond0
+
+		tc -n ${g_ns} qdisc add dev s${i} clsact
 	done
 
 	ip -n ${s_ns} link set bond0 up
-- 
2.38.1

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[kernel test robot]

Hi Hangbin,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Hangbin-Liu/bonding-fix-send_peer_notif-overflow/20230420-162411
patch link:    https://lore.kernel.org/r/20230420082230.2968883-2-liuhangbin%40gmail.com
patch subject: [PATCH net 1/4] bonding: fix send_peer_notif overflow
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20230420/202304202222.eUq4Xfv8-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
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
        # https://github.com/intel-lab-lkp/linux/commit/5bf7296696ea0aa3997bf310fae2aa5cf62a3af5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hangbin-Liu/bonding-fix-send_peer_notif-overflow/20230420-162411
        git checkout 5bf7296696ea0aa3997bf310fae2aa5cf62a3af5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304202222.eUq4Xfv8-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH net 2/4] Documentation: bonding: fix the doc of peer_notif_delay

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Bonding only supports setting peer_notif_delay with miimon set.
>
>Fixes: 0307d589c4d6 ("bonding: add documentation for peer_notif_delay")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> Documentation/networking/bonding.rst | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index adc4bf4f3c50..6daeb18911fb 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -776,10 +776,9 @@ peer_notif_delay
> 	Specify the delay, in milliseconds, between each peer
> 	notification (gratuitous ARP and unsolicited IPv6 Neighbor
> 	Advertisement) when they are issued after a failover event.
>-	This delay should be a multiple of the link monitor interval
>-	(arp_interval or miimon, whichever is active). The default
>-	value is 0 which means to match the value of the link monitor
>-	interval.
>+	This delay should be a multiple of the MII link monitor interval
>+	(miimon). The default value is 0 which means to match the value
>+        of the MII link monitor interval.

	Perhaps something like the following better reflects what we're
trying to convey here?

	This delay is rounded down to the MII link monitor interval
	(miimon), and cannot be set if miimon is not set.  The default
	value is 0 which provides no delay beyond the miimon interval.

	-J

> prio
> 	Slave priority. A higher number means higher priority.
>-- 
>2.38.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Jay Vosburgh]

kernel test robot <lkp@intel.com> wrote:

>Hi Hangbin,
>
>kernel test robot noticed the following build errors:
>
>[auto build test ERROR on net/main]
>
>url:    https://github.com/intel-lab-lkp/linux/commits/Hangbin-Liu/bonding-fix-send_peer_notif-overflow/20230420-162411
>patch link:    https://lore.kernel.org/r/20230420082230.2968883-2-liuhangbin%40gmail.com
>patch subject: [PATCH net 1/4] bonding: fix send_peer_notif overflow
>config: parisc-defconfig (https://download.01.org/0day-ci/archive/20230420/202304202222.eUq4Xfv8-lkp@intel.com/config)
>compiler: hppa-linux-gcc (GCC) 12.1.0
>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
>        # https://github.com/intel-lab-lkp/linux/commit/5bf7296696ea0aa3997bf310fae2aa5cf62a3af5
>        git remote add linux-review https://github.com/intel-lab-lkp/linux
>        git fetch --no-tags linux-review Hangbin-Liu/bonding-fix-send_peer_notif-overflow/20230420-162411
>        git checkout 5bf7296696ea0aa3997bf310fae2aa5cf62a3af5
>        # save the config file
>        mkdir build_dir && cp config build_dir/.config
>        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
>        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash
>
>If you fix the issue, kindly add following tag where applicable
>| Reported-by: kernel test robot <lkp@intel.com>
>| Link: https://lore.kernel.org/oe-kbuild-all/202304202222.eUq4Xfv8-lkp@intel.com/
>
>All errors (new ones prefixed by >>, old ones prefixed by <<):
>
>>> ERROR: modpost: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!

	I assume this is related to send_peer_notif now being u64 in the
modulus at:

static bool bond_should_notify_peers(struct bonding *bond)
{
[...]
        if (!slave || !bond->send_peer_notif ||
            bond->send_peer_notif %
            max(1, bond->params.peer_notif_delay) != 0 ||

	but I'm unsure if this is a real coding error, or some issue
with the parisc arch specifically?

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Jakub Kicinski]

On Thu, 20 Apr 2023 08:59:40 -0700 Jay Vosburgh wrote:
> >All errors (new ones prefixed by >>, old ones prefixed by <<):
> >  
> >>> ERROR: modpost: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!  
> 
> 	I assume this is related to send_peer_notif now being u64 in the
> modulus at:
> 
> static bool bond_should_notify_peers(struct bonding *bond)
> {
> [...]
>         if (!slave || !bond->send_peer_notif ||
>             bond->send_peer_notif %
>             max(1, bond->params.peer_notif_delay) != 0 ||
> 
> 	but I'm unsure if this is a real coding error, or some issue
> with the parisc arch specifically?

Coding error, I think. 
An appropriate helper from linux/math64.h should be used.

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Hangbin Liu]

On Thu, Apr 20, 2023 at 04:21:39PM -0700, Jakub Kicinski wrote:
> On Thu, 20 Apr 2023 08:59:40 -0700 Jay Vosburgh wrote:
> > >All errors (new ones prefixed by >>, old ones prefixed by <<):
> > >  
> > >>> ERROR: modpost: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!  
> > 
> > 	I assume this is related to send_peer_notif now being u64 in the
> > modulus at:
> > 
> > static bool bond_should_notify_peers(struct bonding *bond)
> > {
> > [...]
> >         if (!slave || !bond->send_peer_notif ||
> >             bond->send_peer_notif %
> >             max(1, bond->params.peer_notif_delay) != 0 ||
> > 
> > 	but I'm unsure if this is a real coding error, or some issue
> > with the parisc arch specifically?
> 
> Coding error, I think. 
> An appropriate helper from linux/math64.h should be used.

It looks define send_peer_notif to u64 is a bit too large, which introduce
complex conversion for 32bit arch.

For the remainder operation,
bond->send_peer_notif % max(1, bond->params.peer_notif_delay). u32 % u32 look OK.

But for multiplication operation,
bond->send_peer_notif = bond->params.num_peer_notif * max(1, bond->params.peer_notif_delay);
It's u8 * u32. How about let's limit the peer_notif_delay to less than max(u32 / u8),
then we can just use u32 for send_peer_notif. Is there any realistic meaning
to set peer_notif_delay to max(u32)? I don't think so.

Jay, what do you think?

Thanks
Hangbin

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Thu, Apr 20, 2023 at 04:21:39PM -0700, Jakub Kicinski wrote:
>> On Thu, 20 Apr 2023 08:59:40 -0700 Jay Vosburgh wrote:
>> > >All errors (new ones prefixed by >>, old ones prefixed by <<):
>> > >  
>> > >>> ERROR: modpost: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!  
>> > 
>> > 	I assume this is related to send_peer_notif now being u64 in the
>> > modulus at:
>> > 
>> > static bool bond_should_notify_peers(struct bonding *bond)
>> > {
>> > [...]
>> >         if (!slave || !bond->send_peer_notif ||
>> >             bond->send_peer_notif %
>> >             max(1, bond->params.peer_notif_delay) != 0 ||
>> > 
>> > 	but I'm unsure if this is a real coding error, or some issue
>> > with the parisc arch specifically?
>> 
>> Coding error, I think. 
>> An appropriate helper from linux/math64.h should be used.
>
>It looks define send_peer_notif to u64 is a bit too large, which introduce
>complex conversion for 32bit arch.
>
>For the remainder operation,
>bond->send_peer_notif % max(1, bond->params.peer_notif_delay). u32 % u32 look OK.
>
>But for multiplication operation,
>bond->send_peer_notif = bond->params.num_peer_notif * max(1, bond->params.peer_notif_delay);
>It's u8 * u32. How about let's limit the peer_notif_delay to less than max(u32 / u8),
>then we can just use u32 for send_peer_notif. Is there any realistic meaning
>to set peer_notif_delay to max(u32)? I don't think so.
>
>Jay, what do you think?

	I'm fine to limit the peerf_notif_delay range and then use a
smaller type.

	num_peer_notif is already limited to 255; I'm going to suggest a
limit to the delay of 300 seconds.  That seems like an absurdly long
time for this; I didn't do any kind of science to come up with that
number.

	As peer_notif_delay is stored in units of miimon intervals, that
gives a worst case peer_notif_delay value of 300000 if miimon is 1, and
255 * 300000 fits easily in a u32 for send_peer_notif.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Hangbin Liu]

On Thu, Apr 20, 2023 at 10:13:17PM -0700, Jay Vosburgh wrote:
> >It looks define send_peer_notif to u64 is a bit too large, which introduce
> >complex conversion for 32bit arch.
> >
> >For the remainder operation,
> >bond->send_peer_notif % max(1, bond->params.peer_notif_delay). u32 % u32 look OK.
> >
> >But for multiplication operation,
> >bond->send_peer_notif = bond->params.num_peer_notif * max(1, bond->params.peer_notif_delay);
> >It's u8 * u32. How about let's limit the peer_notif_delay to less than max(u32 / u8),
> >then we can just use u32 for send_peer_notif. Is there any realistic meaning
> >to set peer_notif_delay to max(u32)? I don't think so.
> >
> >Jay, what do you think?
> 
> 	I'm fine to limit the peerf_notif_delay range and then use a
> smaller type.
> 
> 	num_peer_notif is already limited to 255; I'm going to suggest a
> limit to the delay of 300 seconds.  That seems like an absurdly long
> time for this; I didn't do any kind of science to come up with that
> number.
> 
> 	As peer_notif_delay is stored in units of miimon intervals, that
> gives a worst case peer_notif_delay value of 300000 if miimon is 1, and
> 255 * 300000 fits easily in a u32 for send_peer_notif.

OK, I just found another overflow. In bond_fill_info(),
or bond_option_miimon_set():

        if (nla_put_u32(skb, IFLA_BOND_PEER_NOTIF_DELAY,
                        bond->params.peer_notif_delay * bond->params.miimon))
                goto nla_put_failure;

Since both peer_notif_delay and miimon are defined as int, there is a
possibility that the fill in number got overflowed. The same with up/down delay.

Even we limit the peer_notif_delay to 300s, which is 30000, there is still has
possibility got overflowed if we set miimon large enough.

This overflow should only has effect on use space shown since it's a
multiplication result. The kernel part works fine. I'm not sure if we should
also limit the miimon, up/down delay values..

Thanks
Hangbin

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Hangbin Liu]

On Fri, Apr 21, 2023 at 05:55:16PM +0800, Hangbin Liu wrote:
> > 	I'm fine to limit the peerf_notif_delay range and then use a
> > smaller type.
> > 
> > 	num_peer_notif is already limited to 255; I'm going to suggest a
> > limit to the delay of 300 seconds.  That seems like an absurdly long
> > time for this; I didn't do any kind of science to come up with that
> > number.
> > 
> > 	As peer_notif_delay is stored in units of miimon intervals, that
> > gives a worst case peer_notif_delay value of 300000 if miimon is 1, and
> > 255 * 300000 fits easily in a u32 for send_peer_notif.
> 
> OK, I just found another overflow. In bond_fill_info(),
> or bond_option_miimon_set():
> 
>         if (nla_put_u32(skb, IFLA_BOND_PEER_NOTIF_DELAY,
>                         bond->params.peer_notif_delay * bond->params.miimon))
>                 goto nla_put_failure;
> 
> Since both peer_notif_delay and miimon are defined as int, there is a
> possibility that the fill in number got overflowed. The same with up/down delay.
> 
> Even we limit the peer_notif_delay to 300s, which is 30000, there is still has
> possibility got overflowed if we set miimon large enough.
> 
> This overflow should only has effect on use space shown since it's a
> multiplication result. The kernel part works fine. I'm not sure if we should
> also limit the miimon, up/down delay values..

Hi Jay,

Any comments for this issue? Should I send the send_peer_notif fix first and
discuss the miimon, up/down delay userspace overflow issue later?

Thanks
Hangbin

Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Fri, Apr 21, 2023 at 05:55:16PM +0800, Hangbin Liu wrote:
>> > 	I'm fine to limit the peerf_notif_delay range and then use a
>> > smaller type.
>> > 
>> > 	num_peer_notif is already limited to 255; I'm going to suggest a
>> > limit to the delay of 300 seconds.  That seems like an absurdly long
>> > time for this; I didn't do any kind of science to come up with that
>> > number.
>> > 
>> > 	As peer_notif_delay is stored in units of miimon intervals, that
>> > gives a worst case peer_notif_delay value of 300000 if miimon is 1, and
>> > 255 * 300000 fits easily in a u32 for send_peer_notif.
>> 
>> OK, I just found another overflow. In bond_fill_info(),
>> or bond_option_miimon_set():
>> 
>>         if (nla_put_u32(skb, IFLA_BOND_PEER_NOTIF_DELAY,
>>                         bond->params.peer_notif_delay * bond->params.miimon))
>>                 goto nla_put_failure;
>> 
>> Since both peer_notif_delay and miimon are defined as int, there is a
>> possibility that the fill in number got overflowed. The same with up/down delay.
>> 
>> Even we limit the peer_notif_delay to 300s, which is 30000, there is still has
>> possibility got overflowed if we set miimon large enough.
>> 
>> This overflow should only has effect on use space shown since it's a
>> multiplication result. The kernel part works fine. I'm not sure if we should
>> also limit the miimon, up/down delay values..
>
>Hi Jay,
>
>Any comments for this issue? Should I send the send_peer_notif fix first and
>discuss the miimon, up/down delay userspace overflow issue later?

	Let's sort out the current send_peer_notif problems first.  I
don't see that the lack of upper bounds for miimon or up/down delay
causes issues for any reasonable configuration, so it can wait a bit.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com