svn commit: r296428 - head/sys/boot/common
Konstantin Belousov
kostikbel at gmail.com
Mon Mar 7 17:34:18 UTC 2016
On Mon, Mar 07, 2016 at 10:04:59AM -0700, Warner Losh wrote:
> On Mon, Mar 7, 2016 at 9:51 AM, Konstantin Belousov <kostikbel at gmail.com>
> wrote:
>
> > On Mon, Mar 07, 2016 at 09:28:13AM -0700, Warner Losh wrote:
> > > On Mon, Mar 7, 2016 at 8:52 AM, Konstantin Belousov <kostikbel at gmail.com
> > >
> > > wrote:
> > >
> > > > On Mon, Mar 07, 2016 at 08:39:47AM -0700, Ian Lepore wrote:
> > > > > Is there no way to prevent the panic other than making the unwind
> > data
> > > > > be present? Why can't the kernel be fixed to cope with the missing
> > > > > data in some gentler way during a transition period? Perhaps
> > valid-but
> > > > > -fake data could be generated if necessary? Being unable to get a
> > > > > stack traceback through a loaded module would be a small price to pay
> > > > > for trouble-free updgrades.
> > > >
> > > > It is practically impossible to recover from partially-loaded object
> > file'
> > > > module. The loader workaround currently only affects HEAD and since
> > the
> > > > MFC was done, 10.3 should be safe. We always required lastest stable
> > > > for the jump to next major branch.
> > > >
> > > > What could be done is demoting the panics (there are several, besides
> > > > the one which was triggered) to a message and refusing to load the
> > > > affected module. OTOH, if the reaction would be a message and not
> > panic,
> > > > it definitely go ignored for quite some time.
> > > >
> > >
> > > The new loader could also pass in some version or cookie in the metadata
> > > that says it is the new one. The kernel could examine this and issue a
> > > warning,
> > > on amd64 / i386, that module linking may be incomplete and you'll need to
> > > upgrade your /boot/loader if you encounter a crash.
> > This is absolute useless kernel bloat. Kernel should provide an execution
> > environment for user programs, and not lecture users about proper system
> > configuration.
>
>
> On the other hand, the kernel and the boot loader have a protocol they
> both implement. When one side implements it wrong, the other side should
> detect it if it is easy to do so.
>
>
> > > Could the kernel detect that a .eh_frame module was loaded and ignore it
> > > in "safe mode"? Perhaps combined with the new boot-loader cookie, this
> > > would be an automatic way to not mysteriously crash.
> > Why should kernel ignore loaded .eh_frame ? I do not see any use for
> > other part of the suggestion at all. To clarify, kernel paniced because
> > some (required but currently not utilized) part of the binary module was
> > not loaded.
>
>
> Not ignore eh_frame, just modules that have eh_frame and potentially bad
> relocations. Or, you could pre-scan the relocations and only fail when the
> module actually has them. But if you make the linker pancis into warnings
> instead, then that would likely also be OK.
>
> > Alternatively, is there a switch to clang 3.8 that says 'Don't generate
> > the
> > > new
> > > relocation, use the old one instead" which would also be safe and allow a
> > > less-bumpy transition?
> > >
> > > Finally, would the partially loaded module stop at the first bad
> > relocation,
> > > or would it do them all and just skip the bad ones? Is the data from this
> > > relocation
> > > used all the time, or just when we're doing a stack unwind for an
> > exception
> > > or a backtrace?
> > Practically, we could ignore that relocations and still load the module,
> > but this is only because we know what the scope of the relocations is.
> > For some arbitrary situation with the same detected missed place for
> > relocations, loader cannot know is it safe or not.
> >
>
> True. However, this is a well-known case.
There is no way to distinguish well-known case against some other case.
>
>
> > The problem is fixed and does not deserve nuking of all computers in
> > the world, which was an equivalent of some other suggestions how to
> > handle that. Most of the suggestions come to extreme which is not
> > deserved.
> >
> > What could be useful, as I noted already, is to demote the panics from
> > kernel linker to warnings. I intended to work on this.
>
>
> That would fit the bill for what I'm interested in this stuff for. Normally,
> we load the new kernel with new boot loader in my company's
> upgrade process. There are times, however, when we'll wind up
> loading the new kernel with the old boot loader (but more commonly
> vice-versa). Having some indication of the error would be quite useful
> in this scenario so we know we need to do something else.
With the patch attached, and old pxeloader, I get
Preloaded elf obj module "/boot/kernel/aesni.ko" at 0xffffffff80e20478.
kldload: aesni.ko: lost base for reltab
during boot, and then the only consequence is aesni.ko not loaded.
The system booted multiuser.
The error handling is not ideal, some stuff could leak with the patch.
But the same is true for existing error return pathes as well, so I
do not consider this a stopper.
diff --git a/sys/kern/link_elf_obj.c b/sys/kern/link_elf_obj.c
index dfbcdfe..457278d 100644
--- a/sys/kern/link_elf_obj.c
+++ b/sys/kern/link_elf_obj.c
@@ -140,7 +140,7 @@ static int link_elf_each_function_name(linker_file_t,
static int link_elf_each_function_nameval(linker_file_t,
linker_function_nameval_callback_t,
void *);
-static void link_elf_reloc_local(linker_file_t);
+static int link_elf_reloc_local(linker_file_t);
static long link_elf_symtab_get(linker_file_t, const Elf_Sym **);
static long link_elf_strtab_get(linker_file_t, caddr_t *);
@@ -405,15 +405,26 @@ link_elf_link_preload(linker_class_t cls, const char *filename,
break;
}
}
- if (pb != ef->nprogtab)
- panic("lost progbits");
- if (rl != ef->nreltab)
- panic("lost reltab");
- if (ra != ef->nrelatab)
- panic("lost relatab");
+ if (pb != ef->nprogtab) {
+ printf("%s: lost progbits\n", filename);
+ error = ENOEXEC;
+ goto out;
+ }
+ if (rl != ef->nreltab) {
+ printf("%s: lost reltab\n", filename);
+ error = ENOEXEC;
+ goto out;
+ }
+ if (ra != ef->nrelatab) {
+ printf("%s: lost relatab\n", filename);
+ error = ENOEXEC;
+ goto out;
+ }
/* Local intra-module relocations */
- link_elf_reloc_local(lf);
+ error = link_elf_reloc_local(lf);
+ if (error != 0)
+ goto out;
*result = lf;
return (0);
@@ -634,8 +645,11 @@ link_elf_load_file(linker_class_t cls, const char *filename,
ef->relatab = malloc(ef->nrelatab * sizeof(*ef->relatab),
M_LINKER, M_WAITOK | M_ZERO);
- if (symtabindex == -1)
- panic("lost symbol table index");
+ if (symtabindex == -1) {
+ link_elf_error(filename, "lost symbol table index");
+ error = ENOEXEC;
+ goto out;
+ }
/* Allocate space for and load the symbol table */
ef->ddbsymcnt = shdr[symtabindex].sh_size / sizeof(Elf_Sym);
ef->ddbsymtab = malloc(shdr[symtabindex].sh_size, M_LINKER, M_WAITOK);
@@ -650,8 +664,11 @@ link_elf_load_file(linker_class_t cls, const char *filename,
goto out;
}
- if (symstrindex == -1)
- panic("lost symbol string index");
+ if (symstrindex == -1) {
+ link_elf_error(filename, "lost symbol string index");
+ error = ENOEXEC;
+ goto out;
+ }
/* Allocate space for and load the symbol strings */
ef->ddbstrcnt = shdr[symstrindex].sh_size;
ef->ddbstrtab = malloc(shdr[symstrindex].sh_size, M_LINKER, M_WAITOK);
@@ -884,19 +901,34 @@ link_elf_load_file(linker_class_t cls, const char *filename,
break;
}
}
- if (pb != ef->nprogtab)
- panic("lost progbits");
- if (rl != ef->nreltab)
- panic("lost reltab");
- if (ra != ef->nrelatab)
- panic("lost relatab");
- if (mapbase != (vm_offset_t)ef->address + mapsize)
- panic("mapbase 0x%lx != address %p + mapsize 0x%lx (0x%lx)\n",
- (u_long)mapbase, ef->address, (u_long)mapsize,
+ if (pb != ef->nprogtab) {
+ link_elf_error(filename, "lost progbits");
+ error = ENOEXEC;
+ goto out;
+ }
+ if (rl != ef->nreltab) {
+ link_elf_error(filename, "lost reltab");
+ error = ENOEXEC;
+ goto out;
+ }
+ if (ra != ef->nrelatab) {
+ link_elf_error(filename, "lost relatab");
+ error = ENOEXEC;
+ goto out;
+ }
+ if (mapbase != (vm_offset_t)ef->address + mapsize) {
+ printf(
+ "%s: mapbase 0x%lx != address %p + mapsize 0x%lx (0x%lx)\n",
+ filename, (u_long)mapbase, ef->address, (u_long)mapsize,
(u_long)(vm_offset_t)ef->address + mapsize);
+ error = ENOMEM;
+ goto out;
+ }
/* Local intra-module relocations */
- link_elf_reloc_local(lf);
+ error = link_elf_reloc_local(lf);
+ if (error != 0)
+ goto out;
/* Pull in dependencies */
VOP_UNLOCK(nd.ni_vp, 0);
@@ -1034,12 +1066,16 @@ relocate_file(elf_file_t ef)
/* Perform relocations without addend if there are any: */
for (i = 0; i < ef->nreltab; i++) {
rel = ef->reltab[i].rel;
- if (rel == NULL)
- panic("lost a reltab!");
+ if (rel == NULL) {
+ link_elf_error(ef->lf.filename, "lost a reltab!");
+ return (ENOEXEC);
+ }
rellim = rel + ef->reltab[i].nrel;
base = findbase(ef, ef->reltab[i].sec);
- if (base == 0)
- panic("lost base for reltab");
+ if (base == 0) {
+ link_elf_error(ef->lf.filename, "lost base for reltab");
+ return (ENOEXEC);
+ }
for ( ; rel < rellim; rel++) {
symidx = ELF_R_SYM(rel->r_info);
if (symidx >= ef->ddbsymcnt)
@@ -1053,7 +1089,7 @@ relocate_file(elf_file_t ef)
symname = symbol_name(ef, rel->r_info);
printf("link_elf_obj: symbol %s undefined\n",
symname);
- return ENOENT;
+ return (ENOENT);
}
}
}
@@ -1061,12 +1097,17 @@ relocate_file(elf_file_t ef)
/* Perform relocations with addend if there are any: */
for (i = 0; i < ef->nrelatab; i++) {
rela = ef->relatab[i].rela;
- if (rela == NULL)
- panic("lost a relatab!");
+ if (rela == NULL) {
+ link_elf_error(ef->lf.filename, "lost a relatab!");
+ return (ENOEXEC);
+ }
relalim = rela + ef->relatab[i].nrela;
base = findbase(ef, ef->relatab[i].sec);
- if (base == 0)
- panic("lost base for relatab");
+ if (base == 0) {
+ link_elf_error(ef->lf.filename,
+ "lost base for relatab");
+ return (ENOEXEC);
+ }
for ( ; rela < relalim; rela++) {
symidx = ELF_R_SYM(rela->r_info);
if (symidx >= ef->ddbsymcnt)
@@ -1080,7 +1121,7 @@ relocate_file(elf_file_t ef)
symname = symbol_name(ef, rela->r_info);
printf("link_elf_obj: symbol %s undefined\n",
symname);
- return ENOENT;
+ return (ENOENT);
}
}
}
@@ -1092,7 +1133,7 @@ relocate_file(elf_file_t ef)
*/
elf_obj_cleanup_globals_cache(ef);
- return 0;
+ return (0);
}
static int
@@ -1375,7 +1416,7 @@ link_elf_fix_link_set(elf_file_t ef)
}
}
-static void
+static int
link_elf_reloc_local(linker_file_t lf)
{
elf_file_t ef = (elf_file_t)lf;
@@ -1393,12 +1434,16 @@ link_elf_reloc_local(linker_file_t lf)
/* Perform relocations without addend if there are any: */
for (i = 0; i < ef->nreltab; i++) {
rel = ef->reltab[i].rel;
- if (rel == NULL)
- panic("lost a reltab!");
+ if (rel == NULL) {
+ link_elf_error(ef->lf.filename, "lost a reltab");
+ return (ENOEXEC);
+ }
rellim = rel + ef->reltab[i].nrel;
base = findbase(ef, ef->reltab[i].sec);
- if (base == 0)
- panic("lost base for reltab");
+ if (base == 0) {
+ link_elf_error(ef->lf.filename, "lost base for reltab");
+ return (ENOEXEC);
+ }
for ( ; rel < rellim; rel++) {
symidx = ELF_R_SYM(rel->r_info);
if (symidx >= ef->ddbsymcnt)
@@ -1415,12 +1460,16 @@ link_elf_reloc_local(linker_file_t lf)
/* Perform relocations with addend if there are any: */
for (i = 0; i < ef->nrelatab; i++) {
rela = ef->relatab[i].rela;
- if (rela == NULL)
- panic("lost a relatab!");
+ if (rela == NULL) {
+ link_elf_error(ef->lf.filename, "lost a relatab!");
+ return (ENOEXEC);
+ }
relalim = rela + ef->relatab[i].nrela;
base = findbase(ef, ef->relatab[i].sec);
- if (base == 0)
- panic("lost base for relatab");
+ if (base == 0) {
+ link_elf_error(ef->lf.filename, "lost base for reltab");
+ return (ENOEXEC);
+ }
for ( ; rela < relalim; rela++) {
symidx = ELF_R_SYM(rela->r_info);
if (symidx >= ef->ddbsymcnt)
@@ -1433,6 +1482,7 @@ link_elf_reloc_local(linker_file_t lf)
elf_obj_lookup);
}
}
+ return (0);
}
static long
More information about the svn-src-all
mailing list