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

Include interface ID in the PeerAddress that gets handed up from UDP/TCP code #7389

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

bzbarsky-apple
Copy link
Contributor

Problem

When our UDP and TCP code hands a PeerAddress up the stack for incoming messages, it does not include the interface id in that PeerAddress. When upper layers then update their cached PeerAddress based on the incoming address, they can lose the ability to send messages properly (e.g if sending to an IPv6 link-local address).

Change overview

Hand up the right interface ID if we can. For TCP this is half-baked at best and definitely does not work for LwIP.

Testing

Manually tried running /out/debug/standalone/chip-tool onoff toggle 1 when paired to a device that has an IPv6 link-local address. Without these changes the message gets sent but the standalone ack for the response gets "No route to host". With these changes, the ack is also sent correctly.

Without this, we end up sending a message, getting a response,
clobbering our PeerAddress with one without interface info, and then
not being able to send the CRMP standalone ack properly.
@todo
Copy link

todo bot commented Jun 4, 2021

Does netif_get_by_index(mTCP->netif_idx) do the right thing? I

// TODO: Does netif_get_by_index(mTCP->netif_idx) do the right thing? I
// can't quite tell whether LwIP supports a specific interface id for TCP at
// all. For now just claim no particular interface id.
*retInterface = INET_NULL_INTERFACEID;
return INET_NO_ERROR;
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
union
{
sockaddr any;


This comment was generated by todo based on a TODO comment in 9d9cce1 in #7389. cc @bzbarsky-apple.

@todo
Copy link

todo bot commented Jun 4, 2021

Is there still a meaningful interface id in this case?

// TODO: Is there still a meaningful interface id in this case?
*retInterface = INET_NULL_INTERFACEID;
}
return INET_NO_ERROR;
}
#if INET_CONFIG_ENABLE_IPV4
if (sa.any.sa_family == AF_INET)
{
// No interface id available for IPv4 sockets.
*retInterface = INET_NULL_INTERFACEID;


This comment was generated by todo based on a TODO comment in 9d9cce1 in #7389. cc @bzbarsky-apple.

@woody-apple
Copy link
Contributor

@msandstedt @andy31415 @jepenven-silabs ? Would love some eyes on this to unblock testing. Thanks!

@woody-apple
Copy link
Contributor

@saurabhst ?

else
{
// TODO: Is there still a meaningful interface id in this case?
*retInterface = INET_NULL_INTERFACEID;
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, sin6_scope_id will only be an interface ID for link local addresses. So in short, no, I don't believe an interface ID can be extracted for addresses other than link-local. And I can't think of a reason why we would need this for non-link-local addresses. In theory we could bind to a specific interface at which we know a device may reach us. But I don't know why we would want to or need to.

As to the other possible overloaded meanings of sin6_scope_id (zones or site indices?), it's hard to imagine these would be useful to us, or could even be dealt with in a portable way. I don't know much about those though, except that they are not interface indices.

TLDR: It looks to me like the code is doing the correct thing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for looking into this! This part I was really not sure about, as you can tell from the comments. ;)

@bzbarsky-apple bzbarsky-apple deleted the fix-peer-address branch June 4, 2021 17:08
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…TCP code (project-chip#7389)

* Include the interface id in the PeerAddress UDP passes up the stack.

Without this, we end up sending a message, getting a response,
clobbering our PeerAddress with one without interface info, and then
not being able to send the CRMP standalone ack properly.

* Include the interface id in the PeerAddress TCP passes up the stack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants