Support multiple simultaneous mDNS requests

[jukkar]

Fixes #21914

[zephyrbot]

All checks passed.

checkpatch (informational only, not a failure)

-:335: WARNING:INDENTED_LABEL: labels should not be indented
#335: FILE: subsys/net/lib/dns/resolve.c:524:
+		query_known:

- total: 0 errors, 1 warnings, 525 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[jukkar]

Updated according to comments.

[jukkar]

ping @tbursztyka, can you recheck

[pfalcon]

cc @lbise

[pfalcon]

I don't understand the logic behind introducing hash usage here. Hash is needed to speed up comparisons in large search domain. For example, if you have a million entries, you definitely need a hash. How many in-flight requests we'd have here? A dozen? Then, hash has collisions. In other words, if hashes are equal, it doesn't mean that subject datums are equal, that's the purpose of hash - to quickly vet out inequalities, but if hashes are equal, you need to compare the strings themselves. Finally, here crc16 is used as a hash function. Our implementation is optimized for code size, and is thus slow (works on bit level). If we'd have optimization for runtime, it would ship a precomputed value table, which would be mean code size dependency.

So, that's what I see here: using slow hash function -> using it to compare within just a handful of choices -> while doing so, not handling hash collisions. You could instead compare query host name directly, which would be simpler code without additional dependencies, and would be only faster for the realistic cases we have in hand.

I'm sorry if I missed something here.

[jukkar]

I don't understand the logic behind introducing hash usage here. Hash is needed to speed up comparisons in large search domain. For example, if you have a million entries, you definitely need a hash. How many in-flight requests we'd have here? A dozen? Then, hash has collisions. In other words, if hashes are equal, it doesn't mean that subject datums are equal, that's the purpose of hash - to quickly vet out inequalities, but if hashes are equal, you need to compare the strings themselves. Finally, here crc16 is used as a hash function. Our implementation is optimized for code size, and is thus slow (works on bit level). If we'd have optimization for runtime, it would ship a precomputed value table, which would be mean code size dependency.

You missed the problem domain that this PR is trying to solve. So the patch tries to solve the issue of matching sent mDNS request to received reply. Previously we used DNS id for matching the reply but it cannot be used for that purpose in mDNS as the id is 0. So this PR calculates a "hash" which is a crc16 value in this case. I used crc16 as I did not want to re-invent a hash function for this and there was simple API for calculating the crc value. The 16-bit value was used in order not to bloat the dns_resolve_context struct too much. The mDNS resolving is not done many times in a row so speed is not an issue here. Currently crc functions are optimized for code size, when / if we get hw accelerated crc functions there is no possible speed issue.

So, that's what I see here: using slow hash function -> using it to compare within just a handful of choices -> while doing so, not handling hash collisions. You could instead compare query host name directly, which would be simpler code without additional dependencies, and would be only faster for the realistic cases we have in hand.

That was one alternative, it is just that it would bloat things considerably in dns_resolve_context struct. We store pointer to sent query, but it is just a normal string. When receiving the mDNS response, the response is in DNS string format and we cannot easily compare these. We would need to allocate space somewhere etc. which would mean bigger memory consumption.

[pfalcon]

@jukkar

You missed the problem domain that this PR is trying to solve. So the patch tries to solve the issue of matching sent mDNS request to received reply.

Yes, and that's done by matching reply against array of outstanding requests, i.e. searching within the array of outstanding requests, so where's my missing of the problem domain?

I used crc16 as I did not want to re-invent a hash function for this and there was simple API for calculating the crc value.

That's understood, but my comment is that CRC is known not to be an ideal hash function. If you go as far as bringing in a hash function at all, could add one of well-known both-simple-and-fast functions, e.g. "DJB hash": http://www.cse.yorku.ca/~oz/hash.html .

We store pointer to sent query, but it is just a normal string. When receiving the mDNS response, the response is in DNS string format and we cannot easily compare these. We would need to allocate space somewhere etc. which would mean bigger memory consumption.

My point still holds: hash function values collide, so when you get hash match, next step always should be comparing literal (unhashed) content. I don't see that being done, get_slot_by_id() https://github.com/zephyrproject-rtos/zephyr/pull/21952/files#diff-31ea32ed325f4d523a7f77df9b060689R345 is just extended to check hash value (perhaps its name should be changed then?), and it just stops on this match returning the slot index, and as far as I can tell, this return value is used as-is later.

So, you would need to analyze what may happen in the case of hash collision with your solution. I'd say, this opens up a security issue, when request is made to resolve e.g. "foo.com", but reply is used for e.g. "bar.com". This could be done on purpose (collisions for CRC aren't hard to find), and could be used to redirect traffic for one site to another site.

[jukkar]

hash function values collide

Indeed they do. I verified that we do not collide with IPv4 and IPv6 mDNS and DNS queries that we send at the same time (can be checked by samples/net/dns_resolve sample app). This is the most important test here, it is very unlikely that the application would do a query at the same time that would collide with currently pending DNS query. Hash collision is of course possible but IMHO for all the practical purposes the current code is good enough.

[lbise]

I'll test the changes

[lbise]

Tested looks good. Thank you