Page MenuHomeFreeBSD

linuxpki: correct is_{zero,broadcast}_ether_addr
Needs RevisionPublic

Authored by emaste on Jul 23 2019, 3:59 PM.

Details

Reviewers
kib
cem
hselasky
Summary

Broadcast address is all 0xff and zero is all 0x00. Previously these functions tested the sum of the address octets (against 6 * 0xff for broadcast and 0x00 for zero), so for example fa:00:00:00:00:00 would be considered a broadcast address and fa:06:00:00:00:00 would be considered a zero address.

Sponsored by: The FreeBSD Foundation

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

emaste abandoned this revision.Jul 23 2019, 3:59 PM
emaste created this revision.

meh

cem added a comment.Jul 23 2019, 4:17 PM

Why abandon? LGTM if you still want to commit it.

kib added a comment.Jul 23 2019, 4:31 PM

I believe that the arithmetic is performed on the type promoted as unsigned to integer, so really it is not that straightforward.

The functions here don't match the canonical versions in sys/net/ethernet.h and seem unusual, but aren't actually broken since there's no wrapping on the u8 additions. That said I'm still happy to commit this if it seems more standard.

cem added a comment.Jul 23 2019, 4:50 PM

Ah, I see. Well, this change, or a comment, or like a single (int) will at least prevent someone else from scratching their head over the same thing.

kib added a comment.Jul 23 2019, 7:31 PM

I think it is fine to commit. Phab does not allow me to accept the revision.

emaste reclaimed this revision.Jul 23 2019, 7:50 PM
kib accepted this revision.Jul 23 2019, 8:11 PM
This revision is now accepted and ready to land.Jul 23 2019, 8:11 PM
emaste added a subscriber: hselasky.Aug 1 2019, 1:39 PM
hselasky requested changes to this revision.Aug 1 2019, 1:49 PM

Hi,

If the functions are not broken there is no reason to fix them from my POV.

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

Further we want to differentiate from Linux a bit.

#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>

static inline bool
test_ether_addr(const uint8_t * addr)
{
        return ((addr[0] + addr[1] + addr[2] + addr[3] + addr[4] + addr[5]) == 0);
}

                                                                
int main()
{
        static uint8_t addr[6] = { 0xFc, 0x04 };

        printf("%d\n", test_ether_addr(addr));

        return (0);
}
This revision now requires changes to proceed.Aug 1 2019, 1:49 PM

Maybe the following is unclear:

When you compare with a signed integer the summation should also be done integer wise?

cem added a comment.EditedAug 1 2019, 3:05 PM

If the functions are not broken there is no reason to fix them from my POV.

I think it's worth making the implicit int cast explicit, for reader clarity. This wouldn't be a semantic or target code change.

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

This doesn't seem to be the case, depending on SIMD extensions available / used by default. Clang 8.0.0 plain "-O3" generates vectorized code for the AND/OR variants (adding/oring multiple variables at the same time), but NOT the ADD variants: see P282. When -mno-sse is used, like with kernel code, the ADD variants do not change. The non-SIMD AND/OR variants generate slightly smaller code than the ADD variants separately. (With explicit -mavx ... flags, Clang 8.0.0 does generate vectorized ADD as well: P283.)

However, one interesting aspect of the ADD approach is that a caller which checks for both zero addresses and broadcast addresses can save the result of the add, without performing additional memory access or computation (other than cmp 0x5fa): P281. Nothing similarly clever happens when both the AND/OR variants are static inlined.

Further we want to differentiate from Linux a bit.

I don't understand this comment. Can you elaborate? IMO, we want to differentiate from Linux when and only when it is possible to do better than Linux does; not gratuitously.

kib added a comment.Aug 1 2019, 3:13 PM

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

There is no regular (operating on the integer regiter file) instructions on amd64 that could add e.g. three registers once. There is a LEA instruction which allows you to add to registers and a constant, but thats all.

emaste added a comment.Aug 1 2019, 3:32 PM

They are indeed not broken which is why I originally abandoned the review, but reopened it based on cem's feedback - as written there's some opportunity for confusion.

I don't understand this comment. Can you elaborate? IMO, we want to differentiate from Linux when and only when it is possible to do better than Linux does; not gratuitously.

If we can do things differently and performance is not worsened, I think that is good, to avoid people complaining we are just C&P. If we can do things better we should do that.

Your point that the result can be reused for the two functions if we use ADD is a good point.

There is no regular (operating on the integer regiter file) instructions on amd64 that could add e.g. three registers once. There is a LEA instruction which allows you to add to registers and a constant, but thats all.

I thought LEA could do better. My bad.