Skip to content

[FRR]: Fix a BGP check that verifies if the nexthop is invalid#22910

Merged
yejianquan merged 2 commits intosonic-net:masterfrom
cscarpitta:fix/fix_nh_invalid_check
Jul 1, 2025
Merged

[FRR]: Fix a BGP check that verifies if the nexthop is invalid#22910
yejianquan merged 2 commits intosonic-net:masterfrom
cscarpitta:fix/fix_nh_invalid_check

Conversation

@cscarpitta
Copy link
Copy Markdown
Contributor

@cscarpitta cscarpitta commented Jun 10, 2025

Currently, in bgp_update_martian_nexthop we do a check to evaluate whether a nexthop is valid or not. But then the result of this check is not saved and therefore it gets lost.

		if (!ipv4_unicast_valid(&attr->nexthop) ||
			      bgp_nexthop_self(bgp, afi, type, stype, attr, dest));

This commit fixes the issue by saving the result of the check in the nh_invalid variable that already exists.

@cscarpitta
[FRR]: Fix a BGP check that verifies if the nexthop is invalid
9bb7fb6 
Currently, in `bgp_update_martian_nexthop` we do a check to evaluate whether a
nexthop is valid or not. But then the result of this check is not saved and therefore
it gets lost.

```
		if (!ipv4_unicast_valid(&attr->nexthop) ||
			      bgp_nexthop_self(bgp, afi, type, stype, attr, dest));
```

This commit fixes the issue by saving the result of the check in the `nh_invalid` variable
that already exists.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta cscarpitta requested a review from lguohan as a code owner June 10, 2025 07:14
@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 10, 2025

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 10, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@cscarpitta cscarpitta mentioned this pull request Jun 10, 2025
Merged
@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 10, 2025

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 10, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 10, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 13, 2025

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 13, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 13, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 22, 2025

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 22, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 22, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 22, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 22, 2025

Commenter does not have sufficient privileges for PR 22910 in repo sonic-net/sonic-buildimage

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 23, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 23, 2025

Commenter does not have sufficient privileges for PR 22910 in repo sonic-net/sonic-buildimage

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 23, 2025

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 23, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 23, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 23, 2025

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 23, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@cscarpitta
Copy link
Copy Markdown
Contributor Author

cscarpitta commented Jun 24, 2025

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jun 24, 2025

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 24, 2025

Azure Pipelines successfully started running 1 pipeline(s).

BYGX-wcr
BYGX-wcr reviewed Jun 25, 2025
View reviewed changes
Comment thread src/sonic-frr/patch/0002-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch
if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)))
- nh_invalid = (attr->nexthop.s_addr == INADDR_ANY ||
- !ipv4_unicast_valid(&attr->nexthop) ||
+ if (!ipv4_unicast_valid(&attr->nexthop) ||
Copy link
Copy Markdown
Contributor

@BYGX-wcr BYGX-wcr Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we remove the if here?

Copy link
Copy Markdown
Contributor

@BYGX-wcr BYGX-wcr Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cscarpitta, can you please explain this change?

Copy link
Copy Markdown
Contributor Author

@cscarpitta cscarpitta Jun 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @BYGX-wcr I'm fixing an issue in the bgp_update_martian_nexthop() function.

Before my fix:

/* Check if received nexthop is valid or not. */
bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi,
				uint8_t type, uint8_t stype, struct attr *attr,
				struct bgp_dest *dest)
{
	bool nh_invalid = false;

	...

	/* If NEXT_HOP is present, validate it:
	 * The route can have both nexthop + mp_nexthop encoded as multiple NLRIs,
	 * and we MUST check if at least one of them is valid.
	 * E.g.: IPv6 prefix can be with nexthop: 0.0.0.0, and mp_nexthop: fc00::1.
	 */
	if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)))
		if (!ipv4_unicast_valid(&attr->nexthop) ||
			      bgp_nexthop_self(bgp, afi, type, stype, attr, dest));

	/* If MP_NEXTHOP is present, validate it. */
	...

	return nh_invalid;
}

After my fix:

/* Check if received nexthop is valid or not. */
bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi,
				uint8_t type, uint8_t stype, struct attr *attr,
				struct bgp_dest *dest)
{
	bool nh_invalid = false;

	...

	/* If NEXT_HOP is present, validate it:
	 * The route can have both nexthop + mp_nexthop encoded as multiple NLRIs,
	 * and we MUST check if at least one of them is valid.
	 * E.g.: IPv6 prefix can be with nexthop: 0.0.0.0, and mp_nexthop: fc00::1.
	 */
	if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)))
		nh_invalid = !ipv4_unicast_valid(&attr->nexthop) ||
			      bgp_nexthop_self(bgp, afi, type, stype, attr, dest);

	/* If MP_NEXTHOP is present, validate it. */
	...

	return nh_invalid;
}

As you can see, before my fix, the if statement was having no effect, because we were doing the check to evaluate whether the nexthop is valid or not, but then we did not take any action - we ignored the result of the check.

In other words, I am replacing this:

if (!ipv4_unicast_valid(&attr->nexthop) ||
	      bgp_nexthop_self(bgp, afi, type, stype, attr, dest));

with this:

nh_invalid = !ipv4_unicast_valid(&attr->nexthop) ||
			      bgp_nexthop_self(bgp, afi, type, stype, attr, dest);

The function then returns nh_invalid.

Copy link
Copy Markdown
Contributor

@BYGX-wcr BYGX-wcr Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

BYGX-wcr
BYGX-wcr approved these changes Jul 1, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@BYGX-wcr BYGX-wcr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yejianquan yejianquan merged commit 879d0cf into sonic-net:master Jul 1, 2025
20 checks passed
@mssonicbld mssonicbld mentioned this pull request Jul 2, 2025
Merged
@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Jul 2, 2025

Cherry-pick PR to 202505: #23180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BYGX-wcr BYGX-wcr BYGX-wcr approved these changes

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

+1 more reviewer

@ahsalam ahsalam ahsalam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Approved for 202505 Branch Included in 202505 Branch Request for 202505 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cscarpitta @mssonicbld @BYGX-wcr @ahsalam @yejianquan