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

fix(amf): AMF is accepting the PDU Session when a PDU Request with DNN as empty string is been received from UE. #10500

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

sreedharkumartn
Copy link
Contributor

@sreedharkumartn sreedharkumartn commented Nov 23, 2021

Signed-off-by: sreedharkumartn sreedhar.kumar@wavelabs.ai

Summary

Issues handled:

  1. AMF is accepting the PDU-Session when a PDU-Request with DNN as empty string is been received from UE. (Zenhub task ticket [AMF] AMF is accepting the PDU-Session when a PDU-Request with DNN as empty string is been received from UE. #10477).
  2. AMF is sending PDU-Reject with cause 27 for PDU-Release command from UE. (Zenhub task ticket [AMF] AMF is sending PDU-Reject with cause 27 for PDU-Release command from UE. #10479).

Test Plan

Fix for Issue 1.
Subscriber dnn(apn) info:
subscriber

UERANSIM dnn(apn) info:
ueransim_cause_1

Pcap Snap:
Packet_cause1_23rdnov

Log:
mme_cause_1_resolved_23nov.log

Fix for Issue 2.
Pcap Snap:
Packet_cause2_23rdnov

Log:
mme_cause_2_resolved_23nov.log

UT cases Validated:
UT cases

Checked Normal Registration accept, PDU session establishment request and PDU session establishment release with UERANSIM
Pcap Snap:
Packet_23rdnov

Log:
mme_23nov.log

Additional Information

  • This change is backwards-breaking

@sreedharkumartn sreedharkumartn requested review from a team and ulaskozat November 23, 2021 09:08
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Nov 23, 2021
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2021

feg-workflow

    2 files  198 suites   32s ⏱️
342 tests 342 ✔️ 0 💤 0
356 runs  356 ✔️ 0 💤 0

Results for commit e65cdef.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2021

agw-workflow

  28 files    43 suites   3m 34s ⏱️
685 tests 676 ✔️ 9 💤 0
686 runs  677 ✔️ 9 💤 0

Results for commit e65cdef.

♻️ This comment has been updated with latest results.

@@ -593,7 +595,7 @@ M5GSmCause amf_smf_get_smcause(amf_ue_ngap_id_t ue_id, ULNASTransportMsg* msg) {
the external DNN because the DNN was not included
although required or if the DNN could not be resolved.
*/
if (msg->dnn.len == 0 &&
if (msg->dnn.len <= 1 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for this change? should not this be instead msg->dnn.len == 0 || (ue_context->amf_context.apn_config_profile.nb_apns == 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MicrosoftTeams-image (5)
msg->dnn.len is the length of dnn contents. DNN value will have the first byte that tells the DNN length and then DNN value. In this case msg->dnn.len is 1 means no DNN content. Minimum value should be more than 1.

should not this be instead msg->dnn.len == 0 || (ue_context->amf_context.apn_config_profile.nb_apns == 0)
If UE sends empty DNN or DNN IE not present. As per PR #9645 default DNN is picking from apn_config_profile.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about the AND vs OR statement here. Should not it be OR? If there is no apn profile for UE, it should be rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no apn profile for UE and UE sends DNN. As per PR #9645 PDU session will be rejected with cause 91: DNN not supported or not subscribed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
@ulaskozat these are the cases we are handling and their current behavior. Please review it and provide the feedback.
Above the case is for the Cause 27, #9645 PR handling other scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sreedharkumartn where is cause #91 implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause #91 was implemented in lte/gateway/c/core/oai/tasks/amf/amf_smf_send.cpp in PR #9645.
image
image

Copy link
Contributor

@ulaskozat ulaskozat left a comment

Choose a reason for hiding this comment

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

see my comment

@sreedharkumartn sreedharkumartn added ready2merge This PR is ready to be merged (is approved and passes all required checks) and removed ready2merge This PR is ready to be merged (is approved and passes all required checks) labels Nov 25, 2021
…N as empty string is been received from UE.

Signed-off-by: sreedharkumartn <sreedhar.kumar@wavelabs.ai>
Copy link
Contributor

@ulaskozat ulaskozat left a comment

Choose a reason for hiding this comment

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

Approving this. But don't forget to answer the question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue product: 5g sa size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
2 participants