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.

Sign up for GitHub

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

Jump to bottom

net: dns: Fix DNS dispatcher for multiple network interfaces #79588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −17
Open

net: dns: Fix DNS dispatcher for multiple network interfaces #79588

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions subsys/net/lib/dns/dispatcher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,27 +212,31 @@
/* Refuse to register context if we have identical context
* already registered.
*/
if (ctx->type == entry->type &&
ctx->local_addr.sa_family == entry->local_addr.sa_family) {
if (net_sin(&entry->local_addr)->sin_port ==
net_sin(&ctx->local_addr)->sin_port) {
dup = true;
continue;
if (ctx->sock == entry->sock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't explicit check for socket FD make other checks kind of pointless? It's not that you can have a socket that is bound to two different ports...

Copy link
Member

Choose a reason for hiding this comment

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

We should probably check the addresses instead of socket descriptor values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rlubos i think you are right.

@jukkar so we would compare net_sin(&ctx->local_addr)->sin_addr.s_addr == net_sin(&entry->local_addr)->sin_addr.s_addr as well as the port right? And since no two interfaces should have the same IP assigned this would also solve the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this also be called for interfaces that are down or have no address assigned?

Copy link
Member

Choose a reason for hiding this comment

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

we would compare net_sin(&ctx->local_addr)->sin_addr.s_addr == net_sin(&entry->local_addr)->sin_addr.s_addr as well as the port right? And since no two interfaces should have the same IP assigned this would also solve the issue.

It needs to be a bit more complicated as IPv6 needs to be checked too. Your example address check only validates IPv4 address.

Copy link
Member

Choose a reason for hiding this comment

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

Will this also be called for interfaces that are down or have no address assigned?

Please elaborate what you mean, I do not understand your question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this also be called for interfaces that are down or have no address assigned?

Please elaborate what you mean, I do not understand your question.

^ was a misconception of mine and can be ignored

But as far as i can see in the MDNS responder the function register_dispatcher and subsequently dns_dispatcher_register is called for each interface twice (once IPv4 and IPv6) and the local_addr is the same for both interfaces since it is the multicast 224.0.0.251 respectively ff02::fb. So both interfaces would have the same IP during the dispatch_register and the comparison would detect a false duplicate again right?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, the addresses would be the same in this case.

Hmm, I am trying to understand the actual issue. So you see the problem when having two network interfaces, what exactly is printed (if you enable debugging) error / warning etc. in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In short: The second interface was detected as a duplicate by the dispatcher since it had the same address family and port as the first interface (due to MDNS multicast).

@NilsRuf-EH as I will be absent the next few days could you have a look again at what the log output was in detail?

Copy link
Member

Choose a reason for hiding this comment

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

If we have ports and family the same, then to separate them, the network interface is the only left thing that we can use.

if (ctx->type == entry->type &&
ctx->local_addr.sa_family == entry->local_addr.sa_family) {
if (net_sin(&entry->local_addr)->sin_port ==
net_sin(&ctx->local_addr)->sin_port) {
dup = true;

Check notice on line 220 in subsys/net/lib/dns/dispatcher.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

subsys/net/lib/dns/dispatcher.c:220 
-				ctx->local_addr.sa_family == entry->local_addr.sa_family) {
+			    ctx->local_addr.sa_family == entry->local_addr.sa_family) {
 				if (net_sin(&entry->local_addr)->sin_port ==
-					net_sin(&ctx->local_addr)->sin_port) {
+				    net_sin(&ctx->local_addr)->sin_port) {
continue;
}
}
}

/* Then check if there is an entry with same family and
* port already in the list. If there is then we can act
* as a dispatcher for the given socket. Do not break
* from the loop even if we found an entry so that we
* can catch possible duplicates.
/* Then check if there is an entry with same family,
* port and socket already in the list. If there is then
* we can act as a dispatcher for the given socket. Do
* not break from the loop even if we found an entry so
* that we can catch possible duplicates.
*/
if (found == NULL && ctx->type != entry->type &&
ctx->local_addr.sa_family == entry->local_addr.sa_family) {
if (net_sin(&entry->local_addr)->sin_port ==
net_sin(&ctx->local_addr)->sin_port) {
found = entry;
continue;
if (ctx->sock == entry->sock) {
if (found == NULL && ctx->type != entry->type &&
ctx->local_addr.sa_family == entry->local_addr.sa_family) {
if (net_sin(&entry->local_addr)->sin_port ==
net_sin(&ctx->local_addr)->sin_port) {
found = entry;

Check notice on line 237 in subsys/net/lib/dns/dispatcher.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

subsys/net/lib/dns/dispatcher.c:237 
-				ctx->local_addr.sa_family == entry->local_addr.sa_family) {
+			    ctx->local_addr.sa_family == entry->local_addr.sa_family) {
 				if (net_sin(&entry->local_addr)->sin_port ==
-					net_sin(&ctx->local_addr)->sin_port) {
+				    net_sin(&ctx->local_addr)->sin_port) {
continue;
}
}
}
}
Expand Down
Loading