Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider macsec sectag header size in getPortMtu during InitializePort #2789

Merged
merged 1 commit into from
May 31, 2023

Conversation

judyjoseph
Copy link
Contributor

What I did

With macsec profile attached on multiple interfaces, there is a orchagent crash due to SIGABRT seen in Broadcom based devices.

Steps to recreate

configure macsec profile
attach macsec_profile on all interfaces in a linecard
do config reload

May 17 05:14:49.068001 str2--lc1-1 NOTICE swss0#orchagent: :- processQueue: Set buffer queue str2--lc2-1:asic1:Ethernet192:5-6 to BUFFER_PROFILE_TABLE:egress_lossy_profile
May 17 05:14:49.071409 str2--lc1-1 ERR syncd1#syncd: [07:00.0] SAI_API_PORT:brcm_sai_set_port_attribute:1959 Invalid MTU size of 16392 bytes; Max supported MTU size is 16360 bytes.
May 17 05:14:49.071409 str2--lc1-1 ERR syncd1#syncd: :- sendApiResponse: api SAI_COMMON_API_SET failed in syncd mode: SAI_STATUS_INVALID_PARAMETER
May 17 05:14:49.071463 str2--lc1-1 ERR syncd1#syncd: :- processQuadEvent: VID: oid:0x101000000000007 RID: oid:0x100000006
May 17 05:14:49.071463 str2--lc1-1 ERR syncd1#syncd: :- processQuadEvent: attr: SAI_PORT_ATTR_MTU: 16392
May 17 05:14:49.071667 str2--lc1-1 ERR swss1#orchagent: :- set: set status: SAI_STATUS_INVALID_PARAMETER
May 17 05:14:49.071667 str2--lc1-1 ERR swss1#orchagent: :- setPortMtu: Failed to set MTU 16392 to port pid:101000000000007, rv:-5
May 17 05:14:49.071679 str2--lc1-1 ERR swss1#orchagent: :- handleSaiSetStatus: Encountered failure in set operation, exiting orchagent, SAI API: SAI_API_PORT, status: SAI_STATUS_INVALID_PARAMETER

Why I did it

During the InitializePort, on broadcom platforms, the getPortMTU() SAI call returns back the max value supported by ASIC.

This crash in the above logs is due to a race condition were in macsecorch/portsorch tries to set the MTU by adding the MACSEC_SEC_TAGSIZE to this max MTU value resulting in a MTU above what SAI nsupports.

Ideally the port MTU should have been configured to the port MTU ( 9100 ) before macsecmgt/macsecorch gets a chance to update the port MTU to include the SEC_TAG_SIZE of 32 bytes. But if that don't happen and PortsOrch::setMACsecEnabledState is called to enable MACSEC from macsecorch this code (https://github.com/sonic-net/sonic-swss/blob/master/orchagent/portsorch.cpp#L8233) -- will set the MTU as the max MTU returned from SAI + 32 bytes, resulting in SAI_STATUS_INVALID_PARAMETER.

Fix is to reduce the MTU got from SAI during initializePort to take care of MAX_MACSEC_SECTAG_SIZE. We need not check if this is a macsecPort -- it will not be set as a macsecPort by then here during InitializePort is called.

How I verified it
Verified the same steps and no crash seen.

Details if related

@judyjoseph
Copy link
Contributor Author

@dgsudharsan can you take a look at this PR, we are seeing an issue in broadcom based platforms -- where in with macsec when we add the SEC_TAG_HEADER size it goes beyond the MAX MTU supported in SAI and it errors out.
Could you confirm in case of mellanox ASIC/platforms what would the default MTU value retuned when SAI_PORT_ATTR_MTU is called during initializePort()

@dgsudharsan
Copy link
Collaborator

dgsudharsan commented May 25, 2023

Fix is to reduce the MTU got from SAI during initializePort to take care of MAX_MACSEC_SECTAG_SIZE. We need not check if this is a macsecPort -- it will not be set as a macsecPort by then here during InitializePort is called.

We do not have macsec in our platforms. However I am not sure if you need to reduce by default for all scenarios. I prefer skipping setting the MTU when its greater than max allowed MTU and later setting it when configuration is played to have value 9100

@judyjoseph judyjoseph merged commit 1e9a8b7 into sonic-net:master May 31, 2023
@judyjoseph judyjoseph deleted the mtu_fix_macsec branch July 6, 2023 16:42
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants