Page MenuHomeFreeBSD
Differential D19587

net: adjust randomized address bits
ClosedPublic
Actions

Authored by kevans on Mar 15 2019, 2:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 21, 7:21 AM
Unknown Object (File)
Mar 11 2025, 8:42 AM
Unknown Object (File)
Feb 28 2025, 3:16 PM
Unknown Object (File)
Feb 24 2025, 4:54 PM
Unknown Object (File)
Feb 24 2025, 4:49 PM
Unknown Object (File)
Feb 22 2025, 12:50 PM
Unknown Object (File)
Feb 21 2025, 10:11 AM
Unknown Object (File)
Feb 19 2025, 11:57 AM
View All Files
Subscribers
ae
imp
jhb
network

Details

Reviewers
bz
emaste
gnn
shurd
rgrimes
Group Reviewers
iflib
Commits
rS346326: iflib: Use new ether_gen_addr, restricting addresses to that subset
rS346324: net: adjust randomized address bits
Summary

Give 'random' devices a 16-bit allocation out of the FreeBSD Foundation OUI range. Change the name ether_fakeaddr to ether_randomaddr now that we're dealing real MAC addresses with a real OUI rather than random locally-administered addresses.

While here, provide a function for other components generating random addresses to do so by providing a custom mask to ether_randomaddr_lowmask -- given that most things allocating this range will be either deterministic or completely random, this seems fitting.

Move iflib generated-MACs specifically into the 16-bit random allocation here. I'm not against the thought of adding an iflib-specific allocation, but it's clear that the current scheme is against the current guidelines for the OUI.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 23100

Event Timeline

kevans created this revision.Mar 15 2019, 2:25 AM
Herald added a reviewer: shurd. · View Herald TranscriptMar 15 2019, 2:25 AM
Herald added a reviewer: iflib. · View Herald Transcript
Herald added subscribers: ae, imp. · View Herald Transcript
Harbormaster completed remote builds in B23100: Diff 55088.Mar 15 2019, 2:25 AM
imp added a comment.Mar 15 2019, 3:34 AM

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

I think this is a really bad idea and we should not do it.

bz added a comment.Mar 15 2019, 9:44 AM
In D19587#419396, @imp wrote:

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

(a) the fact that we are still not preserving the "randomly generated" addresses across boot is rather a management problem we still have and not a reason not to use OUI space.

(b) bhyve, despite having a block from FreeBSD allocated, is still using the NetAPP OUI in two places, in exactly the same way this will use it for other interfaces. Same problem as (a) there (though there's actually things in place to pass them in on startup, and so does ifconfig ether exist).

(c) the locally administered space is not for random stuff, it is for a locally *administered* space, some local authority who assigns and manages; I have learnt this the hard way by sys-admins telling me off "no DHCPv4 for you", this LA MAC address was not assigned by us, before. They were actually keeping track of each locally administered address.

(d) What you are thinking of is an "unmanaged, I don't care network" where it doesn't even matter if you have conflicts with official OUI space or not. Been there; made that thinking mistake myself.

(e) Using the FreeBSD OUI, like other companies do, gives us a unique space, allows our users to rely on same EUI-48 if they want even in spaces where locally administered is not a thing and I'd hope that now that we centralise the "computation", we'd actually also grow a management part, as writing a rc snippet to save and restore them isn't that hard, we could even have notifications. That way FreeBSD "cloud" deployments or even just lots of FreeBSD machines running these interfaces and bridging them out (also for jail+vnet for example, which is not so much of a difference from say bhyve or the Fusion running on your OSX) can finally stop the admin headache.

(f) for that we should have long time ago carved out some space; if we carve it out anyway, we can as well use it.

(g) the other standards usages of OUIs (I believe NVME also uses the FreeBSD OUI somewhere; maybe that was in bhyve as well to identify the controller vendor), for config spaces, and others, for fixed, special purpose Ethernet MAC address values, exist and are equally valid; some may conflict, others don't, some could possibly even overlap, I guess, without causing any conflicts.

bz added a comment.Mar 15 2019, 10:02 AM

Sorry Kyle, that the full discussion on this is now done on this review. Thanks for starting to consolidate all the "random" uses.
Let's take this slowly and try to get it righter and with rough consensus into something which will (a) sort the various individual implementations, and (b) solve problems for our users.

sys/net/ieee_oui.h
74

I'll leave the comment here; I know I said I prefer "random" to "fake". Now given my longer reply to @imp I think that name is actually also still misleading and I think that's where Warner's comment comes from.

Yes, they are currently "pseudo-randomly generated". I think that's however not the full use case for this range; looking at the range of interfaces: bridge, vxlan, epair, ng_eiface, ... I think we are talking about a range for virtual interfaces here; as I say, I'd really love to grow a way to also preserve the addresses once pseudo-randomly generated.

Can we name this better?

sys/net/if_ethersubr.c
1406

Can we have this grow an "ifp" pointer argument, even if marked __unused for now; that way we will get interface name, possibly vnet context, and ifIndex later should we want to grow a notification and management part on top of this to preserve the EUI-48 addresses?

sys/net/iflib.c
1294

Unrelated: I was wondering how that would not immediately lead to conflicts.
The answer is it's device_get_nameunit() and not just the unit number.

However we could ponder if we would want to try to use something more deterministic, yet not leading to conflicts easily across multiple machines, in ether_*?

1308

Is there any good reason iflib still has its own function and doesn't call into the common ether_ function?

kevans added inline comments.Mar 15 2019, 11:46 AM
sys/net/ieee_oui.h
74

The problem is I intended to also use this space for the relatively small selection of real NICs in (at the very least) ARM-land that do the same thing in places. Some of them can pull from FDT, some of them generate from some EEPROM to match U-Boot, and some of them do this same logic or fallback to it in the event of the above not being possible.

sys/net/if_ethersubr.c
1406

Sure, that sounds like a good plan to me.

sys/net/iflib.c
1308

That reason is entirely that I think wanting to have a deterministic MAC is a good idea if we have enough to go on to derive one. If we could come to an agreement on the best approach to do so in the common functions, I think it'd be a sensible change to make.

imp added a comment.Mar 15 2019, 4:46 PM
In D19587#419445, @bz wrote:
In D19587#419396, @imp wrote:

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

(a) the fact that we are still not preserving the "randomly generated" addresses across boot is rather a management problem we still have and not a reason not to use OUI space.

(b) bhyve, despite having a block from FreeBSD allocated, is still using the NetAPP OUI in two places, in exactly the same way this will use it for other interfaces. Same problem as (a) there (though there's actually things in place to pass them in on startup, and so does ifconfig ether exist).

(c) the locally administered space is not for random stuff, it is for a locally *administered* space, some local authority who assigns and manages; I have learnt this the hard way by sys-admins telling me off "no DHCPv4 for you", this LA MAC address was not assigned by us, before. They were actually keeping track of each locally administered address.

(d) What you are thinking of is an "unmanaged, I don't care network" where it doesn't even matter if you have conflicts with official OUI space or not. Been there; made that thinking mistake myself.

(e) Using the FreeBSD OUI, like other companies do, gives us a unique space, allows our users to rely on same EUI-48 if they want even in spaces where locally administered is not a thing and I'd hope that now that we centralise the "computation", we'd actually also grow a management part, as writing a rc snippet to save and restore them isn't that hard, we could even have notifications. That way FreeBSD "cloud" deployments or even just lots of FreeBSD machines running these interfaces and bridging them out (also for jail+vnet for example, which is not so much of a difference from say bhyve or the Fusion running on your OSX) can finally stop the admin headache.

(f) for that we should have long time ago carved out some space; if we carve it out anyway, we can as well use it.

(g) the other standards usages of OUIs (I believe NVME also uses the FreeBSD OUI somewhere; maybe that was in bhyve as well to identify the controller vendor), for config spaces, and others, for fixed, special purpose Ethernet MAC address values, exist and are equally valid; some may conflict, others don't, some could possibly even overlap, I guess, without causing any conflicts.

I've gone back and found the docs that I read years ago. I think that bz is more correct than I was about these things, so will drop my objection. I do see that this notion was contemplated, even in the docs I had read before, but I'd missed it.

imp added inline comments.Mar 15 2019, 4:54 PM
sys/net/ieee_oui.h
74

We need a good word for "kernel assigned" since that's what we have here. They aren't really random (which is a huge hassle, btw, if they are really random, so I agree with bz there). I agree that random and fake aren't good words here.

I'd suggest ether_generated_addr() and to use GENERATED here.

Though the question becomes do we want two different ones: one that's generated to be unique on the system, and one that's persistent in some way, perhaps generated randomly the first time. If we wanted to do that, and flag the difference in code today, we could have ether_pgen_addr and ether_tgen_addr. p for persistent / permanant and t for temporary / transient. Though 'd' for deterministic might not be bad either for the former.

jhb added a subscriber: jhb.Mar 15 2019, 5:12 PM
jhb added inline comments.
sys/net/if_ethersubr.c
1418

I think what would give more of what bz@ wants would be to pass in the 'ifp' and then use the interface name (not just unit number) as an input to a hash like SHA1 and then use the low bytes of that hash as the input to OUI_FREEBSD. That would give you "random" but deterministic names across boot (so long as the interface name is deterministic). If the kernel is aware of the hostid UUID you could also mix that into the hash input to give values that are deterministic on a given host but different on different hosts.

rgrimes added a subscriber: rgrimes.Mar 15 2019, 5:30 PM
kevans added inline comments.Mar 15 2019, 5:38 PM
sys/net/ieee_oui.h
74

Generated seems to tick all of the boxes that I care about.

sys/net/if_ethersubr.c
1418

That sounds like a reasonable solution for all use-cases. I'll incorporate that into the next version and switch iflib over to using it, since that should also suit iflib's needs.

bz added a subscriber: network.Mar 15 2019, 5:42 PM
bz added inline comments.
sys/net/if_ethersubr.c
1418

@jhb: interface names are not unique on systems. We do have ways (sigh) still to have two interfaces with the same name (and only one visible); it's considered a bug but even if not, interface name + jail name could do it and that's kind of what the host uuid is supposed to be...

I keep wondering how a proper "restore" will work though but that's a next thing I guess.

jhb added inline comments.Mar 15 2019, 6:48 PM
sys/net/if_ethersubr.c
1418

We can add more things to the input of the hash, sure, but a hash with an input derived from the ifp will give you "deterministic" values. The inputs to the hash are the keys that uniquely identify a given interface to avoid conflicts. The other thing to consider is that if people are doing really crazy things on purpose, they can manually assign MAC addresses as an override. We just need to work for common cases out of the box. With this approach there is no save/restore. Instead it makes the generation deterministic.

kevans added inline comments.Mar 15 2019, 7:01 PM
sys/net/if_ethersubr.c
1418

Any strong opinions on MD5 vs. SHA1 given that we only need to come up with three bytes?

jhb added inline comments.Mar 15 2019, 7:25 PM
sys/net/if_ethersubr.c
1418

This isn't a critical path (this happens during device attach, right?), so I don't think the overhead of SHA1 vs MD5 is a concern. I would probably stick with SHA1.

rgrimes added inline comments.Mar 15 2019, 7:42 PM
sys/net/if_ethersubr.c
1418

I agree that sha1 is probably the way to go, we may well wish to remove md5 code in the future and this would be one less place for that effort. md5.s collission surface is 18 bits, below the desired 24 bits wanted, see: https://en.wikipedia.org/wiki/Secure_Hash_Algorithms

kevans updated this revision to Diff 55120.Mar 15 2019, 8:18 PM

Here's a second stab at it. Highlights reel:

  • Changed terminology from random -> generated
  • Threw in some commentary about expectations
  • arc4random -> SHA1 w/ hostuuid and if_xname (to start with...)
  • Converted iflib to using the same mechanism since we're leaning towards "there's nothing wrong with these always being deterministic"
Harbormaster completed remote builds in B23120: Diff 55120.Mar 15 2019, 8:18 PM
kevans added a comment.Mar 28 2019, 2:56 PM

Any further input on this latest iteration?

jhb added inline comments.Mar 28 2019, 8:46 PM
sys/net/if_ethersubr.c
1406

I would still not say "random", maybe something like:

Allocate an address from the FreeBSD Foundation OUI. This uses a cryptographic hash on the containing jail's UUID and the interface name to attempt to provide a unique but stable address. Pseudo-interfaces which require a MAC address should use this function to allocate non-locally-administered addresses.

1413

Do we really need the masked version? I don't see any callers of that in this patchset, only ether_gen_addr? Maybe we should just call this ether_gen_addr and hardcode the mask for now. It can always be changed in the future if a new consumer shows up, but until then I think it just adds noise.

kevans updated this revision to Diff 55562.Mar 28 2019, 9:27 PM
kevans marked 4 inline comments as done.
  • Stripped iflib.c of some headers it no longer needs
  • Pulled out the _masked version of ether_gen_addr; the envisioned other consumed will not be using it, so there is no need.
  • Revised comment to match suggested wording by jhb.
Harbormaster completed remote builds in B23360: Diff 55562.Mar 28 2019, 9:27 PM
kevans added inline comments.Mar 28 2019, 9:28 PM
sys/net/if_ethersubr.c
1406

Whoops, I meant to update that comment. I've stolen your wording verbatim, because it seems sensible to me.

1413

I had intention for another consumer shortly, but then realized that it also didn't deserve its own non-GENERATED category.

bz requested changes to this revision.Mar 31 2019, 10:09 PM
bz added inline comments.
sys/net/if_ethersubr.c
1429

Can you assume that this is always valid? I wonder if this should be something like:

if (td->td_ucred != NULL)
    getcredhostuuid(td->td_ucred, uuid, sizeof(uuid));
This revision now requires changes to proceed.Mar 31 2019, 10:09 PM
jhb added inline comments.Apr 1 2019, 5:45 PM
sys/net/if_ethersubr.c
1429

td_ucred will never be NULL. There is the issue that the UUID might not be set during early boot, but that will still give deterministic results regardless of how the admin configures it. (That is, if you create interfaces during boot-time probe they will use a UUID of all zeroes, but if you use cloned_interfaces in rc.conf or the like you will always use the UUID), so I think the code is fine as-is.

kevans added a comment.Apr 9 2019, 4:15 PM

Any other comments?

kevans added a comment.Apr 12 2019, 4:17 PM

Would iflib like any kind of UPDATING or relnote mention for the generated 'stable' MAC address likely changing (since we're restricting the range now to a subset of the FF OUI), or is it fine without explicit mention?

rgrimes added a comment.Apr 12 2019, 4:26 PM
In D19587#427247, @kevans wrote:

Would iflib like any kind of UPDATING or relnote mention for the generated 'stable' MAC address likely changing (since we're restricting the range now to a subset of the FF OUI), or is it fine without explicit mention?

RE@ would like a release notes entry, this change is important enough to be mentioned as it cleans up some prior headaches.

bz added inline comments.Apr 13 2019, 2:03 PM
sys/net/if_ethersubr.c
1429

In that case just using getcredhostuuid() is probably the right thing.

kevans updated this revision to Diff 56230.Apr 15 2019, 9:41 PM
kevans marked 7 inline comments as done.
Harbormaster completed remote builds in B23683: Diff 56230.Apr 15 2019, 9:41 PM
kevans updated this revision to Diff 56231.Apr 15 2019, 9:43 PM
Harbormaster completed remote builds in B23684: Diff 56231.Apr 15 2019, 9:44 PM
bz accepted this revision as: bz.Apr 15 2019, 10:24 PM

I think this looks fine now. I haven't given it a full read-down or test again (just going by the diff of changes and last comments).

rgrimes accepted this revision.Apr 15 2019, 10:45 PM
rgrimes removed a subscriber: rgrimes.
This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2019, 5:18 PM
Closed by commit rS346324: net: adjust randomized address bits (authored by kevans). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
net/
3 lines
5 lines
2 lines
33 lines
2 lines
7 lines

Diff 55088

View Options

sys/net/ethernet.h

Show First 20 Lines • Show All 1,249 Lines • ▼ Show 20 Linesdigest_dynamic1(Obj_Entry *obj, int early, const Elf_Dyn **dyn_rpath,
case DT_RELASZ: case DT_RELASZ:
obj->relasize = dynp->d_un.d_val; obj->relasize = dynp->d_un.d_val;
break; break;
case DT_RELAENT: case DT_RELAENT:
assert(dynp->d_un.d_val == sizeof(Elf_Rela)); assert(dynp->d_un.d_val == sizeof(Elf_Rela));
break; break;
case DT_RELR:
obj->relr = (const Elf_Relr *)(obj->relocbase + dynp->d_un.d_ptr);
break;
case DT_RELRSZ:
obj->relrsize = dynp->d_un.d_val;
break;
case DT_RELRENT:
assert(dynp->d_un.d_val == sizeof(Elf_Relr));
break;
case DT_PLTREL: case DT_PLTREL:
plttype = dynp->d_un.d_val; plttype = dynp->d_un.d_val;
assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA); assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA);
break; break;
case DT_SYMTAB: case DT_SYMTAB:
obj->symtab = (const Elf_Sym *) obj->symtab = (const Elf_Sym *)
(obj->relocbase + dynp->d_un.d_ptr); (obj->relocbase + dynp->d_un.d_ptr);
▲ Show 20 Lines • Show All 1,877 Lines • ▼ Show 20 Lines if (mprotect(base, sz, prot) == -1) {
obj->path, before ? "en" : "dis", obj->path, before ? "en" : "dis",
rtld_strerror(errno)); rtld_strerror(errno));
return (-1); return (-1);
} }
} }
return (0); return (0);
} }
/* Process RELR relative relocations. */
static void
reloc_relr(Obj_Entry *obj)
{
const Elf_Relr *relr, *relrlim;
kibUnsubmitted
Done
Inline Actions

Please use normal style(9) conventions for the new code. Most important, use tab for indent, not 4 spaces.

kib: Please use normal style(9) conventions for the new code. Most important, use tab for indent…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

OK. I'll use this neovim script for my FreeBSD code:

:so ~/Dev/freebsd/tools/tools/editing/freebsd.vim
:call FreeBSD_Style()
:set ts=8 sw=4 sts=0 noet
fbsd-phab_maskray.me: OK. I'll use this neovim script for my FreeBSD code: ``` :so…
arichardsonUnsubmitted
Not Done
Inline Actions

For most code git-clang-format should be almost correct. However rtld still uses different indentation for some functions...

arichardson: For most code git-clang-format should be almost correct. However rtld still uses different…
Elf_Addr *where;
relrlim = (const Elf_Relr *)((const char *)obj->relr + obj->relrsize);
for (relr = obj->relr; relr < relrlim; relr++) {
Elf_Relr entry = *relr;
kibUnsubmitted
Done
Inline Actions

There should be a blank line after the local declaration.

kib: There should be a blank line after the local declaration.
kibUnsubmitted
Done
Inline Actions

You are checking the lowest bit there, right? I believe & 1 is much more common way to do this.

Initially I was quite confused by the code, but apparently they changed encoding against the initial proposal. Also they made all entries 64bit, right?

IMO the documentation in the tangled thread on some list/google groups is not enough argument to start supporting this stuff. The last activity occured in 2018.

kib: You are checking the lowest bit there, right? I believe `& 1` is much more common way to do…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.lld, llvm-readelf -r, Android bionic, Linux kernel, and Chrome OS.

Someone may need to implement readelf -r for FreeBSD's readelf.

fbsd-phab_maskray.me: Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.
arichardsonUnsubmitted
Not Done
Inline Actions

Or we just switch to MK_LLVM_BINUTILS by default :)

arichardson: Or we just switch to MK_LLVM_BINUTILS by default :)
emasteUnsubmitted
Not Done
Inline Actions

Or both - even when MK_LLVM_BINUTILS lands we presumably will not MFC it, but people may well run readelf from older FreeBSD branches to inspect objects built with RELR.

emaste: Or both - even when `MK_LLVM_BINUTILS` lands we presumably will not MFC it, but people may well…
if ((entry & 1) == 0) {
where = (Elf_Addr *)(obj->relocbase + entry);
*where++ += (Elf_Addr)obj->relocbase;
} else {
for (long i = 0; (entry >>= 1) != 0; i++)
kibUnsubmitted
Done
Inline Actions

If you use local inside block, why not move its declaration into for loop init clause?

kib: If you use local inside block, why not move its declaration into for loop init clause?
if ((entry & 1) != 0)
kibUnsubmitted
Not Done
Inline Actions

!= 0

kib: != 0
where[i] += (Elf_Addr)obj->relocbase;
where += CHAR_BIT * sizeof(Elf_Relr) - 1;
kibUnsubmitted
Done
Inline Actions

What does this 8 mean?

kib: What does this 8 mean?
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

CHAR_BIT = 8. I just switched to CHAR_BIT for clarity.

A bitmap entry describes 63 locations on a 64-bit machine. where skips 63 locations.

fbsd-phab_maskray.me: CHAR_BIT = 8. I just switched to CHAR_BIT for clarity. A bitmap entry describes 63 locations…
}
}
}
/* /*
* Relocate single object. * Relocate single object.
* Returns 0 on success, or -1 on failure. * Returns 0 on success, or -1 on failure.
*/ */
static int static int
relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj, relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
int flags, RtldLockState *lockstate) int flags, RtldLockState *lockstate)
{ {
Show All 10 Linesrelocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
/* There are relocations to the write-protected text segment. */ /* There are relocations to the write-protected text segment. */
if (obj->textrel && reloc_textrel_prot(obj, true) != 0) if (obj->textrel && reloc_textrel_prot(obj, true) != 0)
return (-1); return (-1);
/* Process the non-PLT non-IFUNC relocations. */ /* Process the non-PLT non-IFUNC relocations. */
if (reloc_non_plt(obj, rtldobj, flags, lockstate)) if (reloc_non_plt(obj, rtldobj, flags, lockstate))
return (-1); return (-1);
reloc_relr(obj);
/* Re-protected the text segment. */ /* Re-protected the text segment. */
if (obj->textrel && reloc_textrel_prot(obj, false) != 0) if (obj->textrel && reloc_textrel_prot(obj, false) != 0)
return (-1); return (-1);
/* Set the special PLT or GOT entries. */ /* Set the special PLT or GOT entries. */
init_pltgot(obj); init_pltgot(obj);
▲ Show 20 Lines • Show All 2,902 Lines • Show Last 20 Lines
View Options

sys/net/ieee_oui.h

Show First 20 Lines • Show All 1,249 Lines • ▼ Show 20 Linesdigest_dynamic1(Obj_Entry *obj, int early, const Elf_Dyn **dyn_rpath,
case DT_RELASZ: case DT_RELASZ:
obj->relasize = dynp->d_un.d_val; obj->relasize = dynp->d_un.d_val;
break; break;
case DT_RELAENT: case DT_RELAENT:
assert(dynp->d_un.d_val == sizeof(Elf_Rela)); assert(dynp->d_un.d_val == sizeof(Elf_Rela));
break; break;
case DT_RELR:
obj->relr = (const Elf_Relr *)(obj->relocbase + dynp->d_un.d_ptr);
break;
case DT_RELRSZ:
obj->relrsize = dynp->d_un.d_val;
break;
case DT_RELRENT:
assert(dynp->d_un.d_val == sizeof(Elf_Relr));
break;
case DT_PLTREL: case DT_PLTREL:
plttype = dynp->d_un.d_val; plttype = dynp->d_un.d_val;
assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA); assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA);
break; break;
case DT_SYMTAB: case DT_SYMTAB:
obj->symtab = (const Elf_Sym *) obj->symtab = (const Elf_Sym *)
(obj->relocbase + dynp->d_un.d_ptr); (obj->relocbase + dynp->d_un.d_ptr);
▲ Show 20 Lines • Show All 1,877 Lines • ▼ Show 20 Lines if (mprotect(base, sz, prot) == -1) {
obj->path, before ? "en" : "dis", obj->path, before ? "en" : "dis",
rtld_strerror(errno)); rtld_strerror(errno));
return (-1); return (-1);
} }
} }
return (0); return (0);
} }
/* Process RELR relative relocations. */
static void
reloc_relr(Obj_Entry *obj)
{
const Elf_Relr *relr, *relrlim;
kibUnsubmitted
Done
Inline Actions

Please use normal style(9) conventions for the new code. Most important, use tab for indent, not 4 spaces.

kib: Please use normal style(9) conventions for the new code. Most important, use tab for indent…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

OK. I'll use this neovim script for my FreeBSD code:

:so ~/Dev/freebsd/tools/tools/editing/freebsd.vim
:call FreeBSD_Style()
:set ts=8 sw=4 sts=0 noet
fbsd-phab_maskray.me: OK. I'll use this neovim script for my FreeBSD code: ``` :so…
arichardsonUnsubmitted
Not Done
Inline Actions

For most code git-clang-format should be almost correct. However rtld still uses different indentation for some functions...

arichardson: For most code git-clang-format should be almost correct. However rtld still uses different…
Elf_Addr *where;
relrlim = (const Elf_Relr *)((const char *)obj->relr + obj->relrsize);
for (relr = obj->relr; relr < relrlim; relr++) {
Elf_Relr entry = *relr;
kibUnsubmitted
Done
Inline Actions

There should be a blank line after the local declaration.

kib: There should be a blank line after the local declaration.
kibUnsubmitted
Done
Inline Actions

You are checking the lowest bit there, right? I believe & 1 is much more common way to do this.

Initially I was quite confused by the code, but apparently they changed encoding against the initial proposal. Also they made all entries 64bit, right?

IMO the documentation in the tangled thread on some list/google groups is not enough argument to start supporting this stuff. The last activity occured in 2018.

kib: You are checking the lowest bit there, right? I believe `& 1` is much more common way to do…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.lld, llvm-readelf -r, Android bionic, Linux kernel, and Chrome OS.

Someone may need to implement readelf -r for FreeBSD's readelf.

fbsd-phab_maskray.me: Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.
arichardsonUnsubmitted
Not Done
Inline Actions

Or we just switch to MK_LLVM_BINUTILS by default :)

arichardson: Or we just switch to MK_LLVM_BINUTILS by default :)
emasteUnsubmitted
Not Done
Inline Actions

Or both - even when MK_LLVM_BINUTILS lands we presumably will not MFC it, but people may well run readelf from older FreeBSD branches to inspect objects built with RELR.

emaste: Or both - even when `MK_LLVM_BINUTILS` lands we presumably will not MFC it, but people may well…
if ((entry & 1) == 0) {
where = (Elf_Addr *)(obj->relocbase + entry);
*where++ += (Elf_Addr)obj->relocbase;
} else {
for (long i = 0; (entry >>= 1) != 0; i++)
kibUnsubmitted
Done
Inline Actions

If you use local inside block, why not move its declaration into for loop init clause?

kib: If you use local inside block, why not move its declaration into for loop init clause?
if ((entry & 1) != 0)
kibUnsubmitted
Not Done
Inline Actions

!= 0

kib: != 0
where[i] += (Elf_Addr)obj->relocbase;
where += CHAR_BIT * sizeof(Elf_Relr) - 1;
kibUnsubmitted
Done
Inline Actions

What does this 8 mean?

kib: What does this 8 mean?
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

CHAR_BIT = 8. I just switched to CHAR_BIT for clarity.

A bitmap entry describes 63 locations on a 64-bit machine. where skips 63 locations.

fbsd-phab_maskray.me: CHAR_BIT = 8. I just switched to CHAR_BIT for clarity. A bitmap entry describes 63 locations…
}
}
}
/* /*
* Relocate single object. * Relocate single object.
* Returns 0 on success, or -1 on failure. * Returns 0 on success, or -1 on failure.
*/ */
static int static int
relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj, relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
int flags, RtldLockState *lockstate) int flags, RtldLockState *lockstate)
{ {
Show All 10 Linesrelocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
/* There are relocations to the write-protected text segment. */ /* There are relocations to the write-protected text segment. */
if (obj->textrel && reloc_textrel_prot(obj, true) != 0) if (obj->textrel && reloc_textrel_prot(obj, true) != 0)
return (-1); return (-1);
/* Process the non-PLT non-IFUNC relocations. */ /* Process the non-PLT non-IFUNC relocations. */
if (reloc_non_plt(obj, rtldobj, flags, lockstate)) if (reloc_non_plt(obj, rtldobj, flags, lockstate))
return (-1); return (-1);
reloc_relr(obj);
/* Re-protected the text segment. */ /* Re-protected the text segment. */
if (obj->textrel && reloc_textrel_prot(obj, false) != 0) if (obj->textrel && reloc_textrel_prot(obj, false) != 0)
return (-1); return (-1);
/* Set the special PLT or GOT entries. */ /* Set the special PLT or GOT entries. */
init_pltgot(obj); init_pltgot(obj);
▲ Show 20 Lines • Show All 2,902 Lines • Show Last 20 Lines
View Options

sys/net/if_bridge.c

Show First 20 Lines • Show All 1,249 Lines • ▼ Show 20 Linesdigest_dynamic1(Obj_Entry *obj, int early, const Elf_Dyn **dyn_rpath,
case DT_RELASZ: case DT_RELASZ:
obj->relasize = dynp->d_un.d_val; obj->relasize = dynp->d_un.d_val;
break; break;
case DT_RELAENT: case DT_RELAENT:
assert(dynp->d_un.d_val == sizeof(Elf_Rela)); assert(dynp->d_un.d_val == sizeof(Elf_Rela));
break; break;
case DT_RELR:
obj->relr = (const Elf_Relr *)(obj->relocbase + dynp->d_un.d_ptr);
break;
case DT_RELRSZ:
obj->relrsize = dynp->d_un.d_val;
break;
case DT_RELRENT:
assert(dynp->d_un.d_val == sizeof(Elf_Relr));
break;
case DT_PLTREL: case DT_PLTREL:
plttype = dynp->d_un.d_val; plttype = dynp->d_un.d_val;
assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA); assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA);
break; break;
case DT_SYMTAB: case DT_SYMTAB:
obj->symtab = (const Elf_Sym *) obj->symtab = (const Elf_Sym *)
(obj->relocbase + dynp->d_un.d_ptr); (obj->relocbase + dynp->d_un.d_ptr);
▲ Show 20 Lines • Show All 1,877 Lines • ▼ Show 20 Lines if (mprotect(base, sz, prot) == -1) {
obj->path, before ? "en" : "dis", obj->path, before ? "en" : "dis",
rtld_strerror(errno)); rtld_strerror(errno));
return (-1); return (-1);
} }
} }
return (0); return (0);
} }
/* Process RELR relative relocations. */
static void
reloc_relr(Obj_Entry *obj)
{
const Elf_Relr *relr, *relrlim;
kibUnsubmitted
Done
Inline Actions

Please use normal style(9) conventions for the new code. Most important, use tab for indent, not 4 spaces.

kib: Please use normal style(9) conventions for the new code. Most important, use tab for indent…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

OK. I'll use this neovim script for my FreeBSD code:

:so ~/Dev/freebsd/tools/tools/editing/freebsd.vim
:call FreeBSD_Style()
:set ts=8 sw=4 sts=0 noet
fbsd-phab_maskray.me: OK. I'll use this neovim script for my FreeBSD code: ``` :so…
arichardsonUnsubmitted
Not Done
Inline Actions

For most code git-clang-format should be almost correct. However rtld still uses different indentation for some functions...

arichardson: For most code git-clang-format should be almost correct. However rtld still uses different…
Elf_Addr *where;
relrlim = (const Elf_Relr *)((const char *)obj->relr + obj->relrsize);
for (relr = obj->relr; relr < relrlim; relr++) {
Elf_Relr entry = *relr;
kibUnsubmitted
Done
Inline Actions

There should be a blank line after the local declaration.

kib: There should be a blank line after the local declaration.
kibUnsubmitted
Done
Inline Actions

You are checking the lowest bit there, right? I believe & 1 is much more common way to do this.

Initially I was quite confused by the code, but apparently they changed encoding against the initial proposal. Also they made all entries 64bit, right?

IMO the documentation in the tangled thread on some list/google groups is not enough argument to start supporting this stuff. The last activity occured in 2018.

kib: You are checking the lowest bit there, right? I believe `& 1` is much more common way to do…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.lld, llvm-readelf -r, Android bionic, Linux kernel, and Chrome OS.

Someone may need to implement readelf -r for FreeBSD's readelf.

fbsd-phab_maskray.me: Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.
arichardsonUnsubmitted
Not Done
Inline Actions

Or we just switch to MK_LLVM_BINUTILS by default :)

arichardson: Or we just switch to MK_LLVM_BINUTILS by default :)
emasteUnsubmitted
Not Done
Inline Actions

Or both - even when MK_LLVM_BINUTILS lands we presumably will not MFC it, but people may well run readelf from older FreeBSD branches to inspect objects built with RELR.

emaste: Or both - even when `MK_LLVM_BINUTILS` lands we presumably will not MFC it, but people may well…
if ((entry & 1) == 0) {
where = (Elf_Addr *)(obj->relocbase + entry);
*where++ += (Elf_Addr)obj->relocbase;
} else {
for (long i = 0; (entry >>= 1) != 0; i++)
kibUnsubmitted
Done
Inline Actions

If you use local inside block, why not move its declaration into for loop init clause?

kib: If you use local inside block, why not move its declaration into for loop init clause?
if ((entry & 1) != 0)
kibUnsubmitted
Not Done
Inline Actions

!= 0

kib: != 0
where[i] += (Elf_Addr)obj->relocbase;
where += CHAR_BIT * sizeof(Elf_Relr) - 1;
kibUnsubmitted
Done
Inline Actions

What does this 8 mean?

kib: What does this 8 mean?
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

CHAR_BIT = 8. I just switched to CHAR_BIT for clarity.

A bitmap entry describes 63 locations on a 64-bit machine. where skips 63 locations.

fbsd-phab_maskray.me: CHAR_BIT = 8. I just switched to CHAR_BIT for clarity. A bitmap entry describes 63 locations…
}
}
}
/* /*
* Relocate single object. * Relocate single object.
* Returns 0 on success, or -1 on failure. * Returns 0 on success, or -1 on failure.
*/ */
static int static int
relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj, relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
int flags, RtldLockState *lockstate) int flags, RtldLockState *lockstate)
{ {
Show All 10 Linesrelocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
/* There are relocations to the write-protected text segment. */ /* There are relocations to the write-protected text segment. */
if (obj->textrel && reloc_textrel_prot(obj, true) != 0) if (obj->textrel && reloc_textrel_prot(obj, true) != 0)
return (-1); return (-1);
/* Process the non-PLT non-IFUNC relocations. */ /* Process the non-PLT non-IFUNC relocations. */
if (reloc_non_plt(obj, rtldobj, flags, lockstate)) if (reloc_non_plt(obj, rtldobj, flags, lockstate))
return (-1); return (-1);
reloc_relr(obj);
/* Re-protected the text segment. */ /* Re-protected the text segment. */
if (obj->textrel && reloc_textrel_prot(obj, false) != 0) if (obj->textrel && reloc_textrel_prot(obj, false) != 0)
return (-1); return (-1);
/* Set the special PLT or GOT entries. */ /* Set the special PLT or GOT entries. */
init_pltgot(obj); init_pltgot(obj);
▲ Show 20 Lines • Show All 2,902 Lines • Show Last 20 Lines
View Options

sys/net/if_ethersubr.c

Show First 20 Lines • Show All 1,249 Lines • ▼ Show 20 Linesdigest_dynamic1(Obj_Entry *obj, int early, const Elf_Dyn **dyn_rpath,
case DT_RELASZ: case DT_RELASZ:
obj->relasize = dynp->d_un.d_val; obj->relasize = dynp->d_un.d_val;
break; break;
case DT_RELAENT: case DT_RELAENT:
assert(dynp->d_un.d_val == sizeof(Elf_Rela)); assert(dynp->d_un.d_val == sizeof(Elf_Rela));
break; break;
case DT_RELR:
obj->relr = (const Elf_Relr *)(obj->relocbase + dynp->d_un.d_ptr);
break;
case DT_RELRSZ:
obj->relrsize = dynp->d_un.d_val;
break;
case DT_RELRENT:
assert(dynp->d_un.d_val == sizeof(Elf_Relr));
break;
case DT_PLTREL: case DT_PLTREL:
plttype = dynp->d_un.d_val; plttype = dynp->d_un.d_val;
assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA); assert(dynp->d_un.d_val == DT_REL || plttype == DT_RELA);
break; break;
case DT_SYMTAB: case DT_SYMTAB:
obj->symtab = (const Elf_Sym *) obj->symtab = (const Elf_Sym *)
(obj->relocbase + dynp->d_un.d_ptr); (obj->relocbase + dynp->d_un.d_ptr);
▲ Show 20 Lines • Show All 1,877 Lines • ▼ Show 20 Lines if (mprotect(base, sz, prot) == -1) {
obj->path, before ? "en" : "dis", obj->path, before ? "en" : "dis",
rtld_strerror(errno)); rtld_strerror(errno));
return (-1); return (-1);
} }
} }
return (0); return (0);
} }
/* Process RELR relative relocations. */
static void
reloc_relr(Obj_Entry *obj)
{
const Elf_Relr *relr, *relrlim;
kibUnsubmitted
Done
Inline Actions

Please use normal style(9) conventions for the new code. Most important, use tab for indent, not 4 spaces.

kib: Please use normal style(9) conventions for the new code. Most important, use tab for indent…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

OK. I'll use this neovim script for my FreeBSD code:

:so ~/Dev/freebsd/tools/tools/editing/freebsd.vim
:call FreeBSD_Style()
:set ts=8 sw=4 sts=0 noet
fbsd-phab_maskray.me: OK. I'll use this neovim script for my FreeBSD code: ``` :so…
arichardsonUnsubmitted
Not Done
Inline Actions

For most code git-clang-format should be almost correct. However rtld still uses different indentation for some functions...

arichardson: For most code git-clang-format should be almost correct. However rtld still uses different…
Elf_Addr *where;
relrlim = (const Elf_Relr *)((const char *)obj->relr + obj->relrsize);
for (relr = obj->relr; relr < relrlim; relr++) {
Elf_Relr entry = *relr;
kibUnsubmitted
Done
Inline Actions

There should be a blank line after the local declaration.

kib: There should be a blank line after the local declaration.
kibUnsubmitted
Done
Inline Actions

You are checking the lowest bit there, right? I believe & 1 is much more common way to do this.

Initially I was quite confused by the code, but apparently they changed encoding against the initial proposal. Also they made all entries 64bit, right?

IMO the documentation in the tangled thread on some list/google groups is not enough argument to start supporting this stuff. The last activity occured in 2018.

kib: You are checking the lowest bit there, right? I believe `& 1` is much more common way to do…
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.lld, llvm-readelf -r, Android bionic, Linux kernel, and Chrome OS.

Someone may need to implement readelf -r for FreeBSD's readelf.

fbsd-phab_maskray.me: Yes, the format changed. Ali Bahrami's comment (scroll to the bottom) is the format used by ld.
arichardsonUnsubmitted
Not Done
Inline Actions

Or we just switch to MK_LLVM_BINUTILS by default :)

arichardson: Or we just switch to MK_LLVM_BINUTILS by default :)
emasteUnsubmitted
Not Done
Inline Actions

Or both - even when MK_LLVM_BINUTILS lands we presumably will not MFC it, but people may well run readelf from older FreeBSD branches to inspect objects built with RELR.

emaste: Or both - even when `MK_LLVM_BINUTILS` lands we presumably will not MFC it, but people may well…
if ((entry & 1) == 0) {
where = (Elf_Addr *)(obj->relocbase + entry);
*where++ += (Elf_Addr)obj->relocbase;
} else {
for (long i = 0; (entry >>= 1) != 0; i++)
kibUnsubmitted
Done
Inline Actions

If you use local inside block, why not move its declaration into for loop init clause?

kib: If you use local inside block, why not move its declaration into for loop init clause?
if ((entry & 1) != 0)
kibUnsubmitted
Not Done
Inline Actions

!= 0

kib: != 0
where[i] += (Elf_Addr)obj->relocbase;
where += CHAR_BIT * sizeof(Elf_Relr) - 1;
kibUnsubmitted
Done
Inline Actions

What does this 8 mean?

kib: What does this 8 mean?
fbsd-phab_maskray.meAuthorUnsubmitted
Done
Inline Actions

CHAR_BIT = 8. I just switched to CHAR_BIT for clarity.

A bitmap entry describes 63 locations on a 64-bit machine. where skips 63 locations.

fbsd-phab_maskray.me: CHAR_BIT = 8. I just switched to CHAR_BIT for clarity. A bitmap entry describes 63 locations…
}
}
}
/* /*
* Relocate single object. * Relocate single object.
* Returns 0 on success, or -1 on failure. * Returns 0 on success, or -1 on failure.
*/ */
static int static int
relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj, relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
int flags, RtldLockState *lockstate) int flags, RtldLockState *lockstate)
{ {
Show All 10 Linesrelocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
/* There are relocations to the write-protected text segment. */ /* There are relocations to the write-protected text segment. */
if (obj->textrel && reloc_textrel_prot(obj, true) != 0) if (obj->textrel && reloc_textrel_prot(obj, true) != 0)
return (-1); return (-1);
/* Process the non-PLT non-IFUNC relocations. */ /* Process the non-PLT non-IFUNC relocations. */
if (reloc_non_plt(obj, rtldobj, flags, lockstate)) if (reloc_non_plt(obj, rtldobj, flags, lockstate))
return (-1); return (-1);
reloc_relr(obj);
/* Re-protected the text segment. */ /* Re-protected the text segment. */
if (obj->textrel && reloc_textrel_prot(obj, false) != 0) if (obj->textrel && reloc_textrel_prot(obj, false) != 0)
return (-1); return (-1);
/* Set the special PLT or GOT entries. */ /* Set the special PLT or GOT entries. */
init_pltgot(obj); init_pltgot(obj);
▲ Show 20 Lines • Show All 2,902 Lines • Show Last 20 Lines
View Options

sys/net/if_vxlan.c

Loading...
View Options

sys/net/iflib.c

Loading...