tcp: restrict flowtype copying to specific RSS TCP types

[cc]

Summary

Recent driver fixes like "20285cad7a55", "efcc0423d80e" and "21dd554d1697" reveal that the m_pkthdr.rsstype shall be more strictly checked before accepting RSS values for inp_flowid.

[gallatin]

Slightly worried some NIC somwhere might not be setting this when it should. Is this solving a problem for you?

[cc]

In D55196#1262206, @gallatin wrote:

Slightly worried some NIC somwhere might not be setting this when it should. Is this solving a problem for you?

The "Test Plan" section from https://reviews.freebsd.org/D55137 shows that M_HASHTYPE_OPAQUE pseudo-hash values can be accepted, in contrast.

[kbowling]

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

[tuexen]

If we have some rules regarding how to set the hash type, can we document them somewhere? This way driver writers know what to do and we can inspect drivers and fix them.

[kbowling]

In D55196#1262240, @tuexen wrote:

If we have some rules regarding how to set the hash type, can we document them somewhere? This way driver writers know what to do and we can inspect drivers and fix them.

I'm basing it off https://cgit.freebsd.org/src/tree/sys/sys/mbuf.h#n527

[cc]

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

I think it's OK to have pseudo hash, e.g. if_vtnet.c, for the driver's own purpose (like "ordering"). But it is incorrect for TCP, as inp_flowid in TCP should be a 4-tuple hash value.

[gallatin]

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

[kbowling]

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Ok.

Separately, this was noticed because some of the drivers are setting opaque when there is no hash value, I think all the intel drivers have this behavior and only a couple have been patched. The intel docs are pretty clear that they implement the Microsoft hashing or it will be empty. I think there might be a few other drivers with similar issue(s).

[tuexen]

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Question: Is M_HASHTYPE_RSS_IPV4 or M_HASHTYPE_RSS_IPV6 (a hash only on the src/dst address) acceptable or not? M_HASHTYPE_ISHASH() is true for them.

[gallatin]

In D55196#1262351, @tuexen wrote:

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Question: Is M_HASHTYPE_RSS_IPV4 or M_HASHTYPE_RSS_IPV6 (a hash only on the src/dst address) acceptable or not? M_HASHTYPE_ISHASH() is true for them.

It should never happen. But to be safe, maybe make a new M_HASHTYPE_ISHASH_TCP ? At this point, my only objection is to the ugly comparisons :)

[tuexen]

In D55196#1262391, @gallatin wrote:

In D55196#1262351, @tuexen wrote:

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Question: Is M_HASHTYPE_RSS_IPV4 or M_HASHTYPE_RSS_IPV6 (a hash only on the src/dst address) acceptable or not? M_HASHTYPE_ISHASH() is true for them.

It should never happen. But to be safe, maybe make a new M_HASHTYPE_ISHASH_TCP ? At this point, my only objection is to the ugly comparisons :)

I totally agree. If we want such a hash, we should check for it.

[cc]

In D55196#1262402, @tuexen wrote:

In D55196#1262391, @gallatin wrote:

In D55196#1262351, @tuexen wrote:

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Question: Is M_HASHTYPE_RSS_IPV4 or M_HASHTYPE_RSS_IPV6 (a hash only on the src/dst address) acceptable or not? M_HASHTYPE_ISHASH() is true for them.

It should never happen. But to be safe, maybe make a new M_HASHTYPE_ISHASH_TCP ? At this point, my only objection is to the ugly comparisons :)

I totally agree. If we want such a hash, we should check for it.

Looks like a common ground here :) I will work on the idea M_HASHTYPE_ISHASH_TCP. thanks

[cc]

1. cancel the unnecessary change to sc_flowtype check in syncache_socket()
2. for the rest, check RSS types by a new macro M_HASHTYPE_ISHASH_TCP

[gallatin]

Looks good. Thank you!