Bug 259528 - [carp] Backup node becomes second active master when master have some negative demotion values
Summary: [carp] Backup node becomes second active master when master have some negativ...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Kristof Provost
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-29 12:58 UTC by Marius Halden
Modified: 2021-11-22 05:18 UTC (History)
1 user (show)

See Also:


Attachments
reproduction.sh (1.75 KB, text/plain)
2021-10-29 12:58 UTC, Marius Halden
no flags Details
carp-negative-demote.patch (763 bytes, patch)
2021-10-29 13:04 UTC, Marius Halden
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marius Halden 2021-10-29 12:58:37 UTC
Created attachment 229121 [details]
reproduction.sh

Given nodes 1 and 2, where node 1 has an advskew of 0 and node 2 has an advskew of 100, making them master and backup respectively.

If net.inet.carp.demotion is set to a negative value on node 1, node 2 might become master while node 1 still retains it master status. Wether or not node 2 becomes master seems to depend on the nodes advskew and what the demotion sysctl was set to on node 1.

The reason for node 2 becoming master seems to be that the calculated advskew taking demotion into account is truncated to a single unsigned byte when copied into the carp header for sending[0], and node 1 stays master since it takes uses the whole non-truncated calculated advskew when deciding wether to stay master[1].

To reproduce, make sure carp support is enabled (kernel module loaded or built in), then run attached reproduction case.

Expected output from reproduction script when setting a negative demotion on the master:
># sh carp-test.sh
>Staring jail carp1.
>Staring jail carp2.
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: BACKUP
>
>decreasing carp demotion in carp1 jail
>net.inet.carp.demotion: 0 -> -1
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: BACKUP
>
>increasing carp demotion in carp1 jail
>net.inet.carp.demotion: -1 -> 0
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: BACKUP
>
>Cleaning up.

Output actually seen when running the script:
># sh carp-test.sh
>Staring jail carp1.
>Staring jail carp2.
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: BACKUP
>
>decreasing carp demotion in carp1 jail
>net.inet.carp.demotion: 0 -> -1
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: MASTER
>
>increasing carp demotion in carp1 jail
>net.inet.carp.demotion: -1 -> 0
>
>Waiting for things to settle...
>
>carp1 status: MASTER
>carp2 status: BACKUP
>
>Cleaning up.

As shown, both carp nodes become master at the same time.

[0] https://cgit.freebsd.org/src/tree/sys/netinet/ip_carp.c?id=130aebbab0d38da85f7d32b6d4227f95a2cd9ec7#n931
[1] https://cgit.freebsd.org/src/tree/sys/netinet/ip_carp.c?id=130aebbab0d38da85f7d32b6d4227f95a2cd9ec7#n736
Comment 1 Marius Halden 2021-10-29 13:04:38 UTC
Created attachment 229122 [details]
carp-negative-demote.patch

The attached patch solves the issue by not allowing the demoted advskew to be less than zero.
Comment 2 Kristof Provost freebsd_committer freebsd_triage 2021-10-31 21:26:08 UTC
https://reviews.freebsd.org/D32759 and https://reviews.freebsd.org/D32760

Seems reasonable.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-11-01 17:09:25 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1019354b54161c140d10a8203ff1e849c09c8010

commit 1019354b54161c140d10a8203ff1e849c09c8010
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:18:42 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-01 16:08:23 +0000

    carp: deal with negative net.inet.carp.demotion

    Given nodes 1 and 2, where node 1 has an advskew of 0 and node 2 has an
    advskew of 100, making them master and backup respectively.

    If net.inet.carp.demotion is set to a negative value on node 1, node 2
    might become master while node 1 still retains it master status. Wether
    or not node 2 becomes master seems to depend on the nodes advskew and
    what the demotion sysctl was set to on node 1.

    The reason for node 2 becoming master seems to be that the calculated
    advskew taking demotion into account is truncated to a single unsigned
    byte when copied into the carp header for sending, and node 1 stays
    master since it takes uses the whole non-truncated calculated advskew
    when deciding wether to stay master.

    PR:             259528
    Reviewed by:    donner, glebius
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32759

 sys/netinet/ip_carp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-11-01 17:09:29 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=847b0d07c43c1190f0015c4c93a9e8fd953e3ab5

commit 847b0d07c43c1190f0015c4c93a9e8fd953e3ab5
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:22:10 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-01 16:08:23 +0000

    carp tests: negative demotion

    PR:             259528
    Reviewed by:    donner
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32760

 tests/sys/netinet/carp.sh | 67 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 8 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-11-22 05:17:09 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d42231181626b2fe0f75514c9b5753083cdf362b

commit d42231181626b2fe0f75514c9b5753083cdf362b
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:22:10 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-22 01:58:10 +0000

    carp tests: negative demotion

    PR:             259528
    Reviewed by:    donner
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32760

    (cherry picked from commit 847b0d07c43c1190f0015c4c93a9e8fd953e3ab5)

 tests/sys/netinet/carp.sh | 67 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 8 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-11-22 05:17:12 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=0909e05779eed325fa0c2d4c1daa6916dbbc83e3

commit 0909e05779eed325fa0c2d4c1daa6916dbbc83e3
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:18:42 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-22 01:55:02 +0000

    carp: deal with negative net.inet.carp.demotion

    Given nodes 1 and 2, where node 1 has an advskew of 0 and node 2 has an
    advskew of 100, making them master and backup respectively.

    If net.inet.carp.demotion is set to a negative value on node 1, node 2
    might become master while node 1 still retains it master status. Wether
    or not node 2 becomes master seems to depend on the nodes advskew and
    what the demotion sysctl was set to on node 1.

    The reason for node 2 becoming master seems to be that the calculated
    advskew taking demotion into account is truncated to a single unsigned
    byte when copied into the carp header for sending, and node 1 stays
    master since it takes uses the whole non-truncated calculated advskew
    when deciding wether to stay master.

    PR:             259528
    Reviewed by:    donner, glebius
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32759

    (cherry picked from commit 1019354b54161c140d10a8203ff1e849c09c8010)

 sys/netinet/ip_carp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-11-22 05:17:13 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b6237b80891f1a6e9808f630a4626eab6ea203bc

commit b6237b80891f1a6e9808f630a4626eab6ea203bc
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:22:10 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-22 01:55:07 +0000

    carp tests: negative demotion

    PR:             259528
    Reviewed by:    donner
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32760

    (cherry picked from commit 847b0d07c43c1190f0015c4c93a9e8fd953e3ab5)

 tests/sys/netinet/carp.sh | 67 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 8 deletions(-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2021-11-22 05:17:13 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1c16de99bd7ddddad9d5877ba0f91aa2a2f57eb3

commit 1c16de99bd7ddddad9d5877ba0f91aa2a2f57eb3
Author:     Marius Halden <marius.halden@modirum.com>
AuthorDate: 2021-10-31 20:18:42 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-22 01:55:13 +0000

    carp: deal with negative net.inet.carp.demotion

    Given nodes 1 and 2, where node 1 has an advskew of 0 and node 2 has an
    advskew of 100, making them master and backup respectively.

    If net.inet.carp.demotion is set to a negative value on node 1, node 2
    might become master while node 1 still retains it master status. Wether
    or not node 2 becomes master seems to depend on the nodes advskew and
    what the demotion sysctl was set to on node 1.

    The reason for node 2 becoming master seems to be that the calculated
    advskew taking demotion into account is truncated to a single unsigned
    byte when copied into the carp header for sending, and node 1 stays
    master since it takes uses the whole non-truncated calculated advskew
    when deciding wether to stay master.

    PR:             259528
    Reviewed by:    donner, glebius
    MFC after:      3 weeks
    Sponsored by:   Modirum MDPay
    Differential Revision:  https://reviews.freebsd.org/D32759

    (cherry picked from commit 1019354b54161c140d10a8203ff1e849c09c8010)

 sys/netinet/ip_carp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)