Page MenuHomeFreeBSD
Differential D13923

Fix kldxref failing for modules with a short mc_cval
ClosedPublic
Actions

Authored by mhorne063_gmail.com on Jan 15 2018, 7:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 20, 7:20 PM
Unknown Object (File)
Fri, Apr 18, 7:45 AM
Unknown Object (File)
Sun, Apr 13, 12:11 PM
Unknown Object (File)
Sun, Apr 13, 1:08 AM
Unknown Object (File)
Mon, Apr 7, 1:22 PM
Unknown Object (File)
Mon, Apr 7, 7:31 AM
Unknown Object (File)
Mar 1 2025, 4:38 AM
Unknown Object (File)
Feb 28 2025, 6:09 AM
View All 91 Files
Subscribers
cem
imp
jhb
kib

Details

Reviewers
emaste
kib
cem
Commits
rS328052: kldxref: handle modules with md_cval at the end of allocated sections
Summary

Attempting to retrieve an md_cval string from a kernel module with
kldxref would throw a offset error for modules created using lld, since
this value would be placed at the end of all allocated sections.

Add an ef_read_seg_string method to the ef interface, to allow reading
strings of varying size without attempting to read beyond the segment's
bounds.

Bugzilla

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mhorne063_gmail.com created this revision.Jan 15 2018, 7:21 PM
mhorne063_gmail.com edited the summary of this revision. (Show Details)
mhorne063_gmail.com edited the summary of this revision. (Show Details)
mhorne063_gmail.com edited the summary of this revision. (Show Details)Jan 15 2018, 7:25 PM
emaste added a subscriber: jhb.Jan 15 2018, 7:26 PM
mhorne063_gmail.com updated this revision to Diff 37990.Jan 15 2018, 7:56 PM

Upload diff correctly with tabs and full context

cem added a subscriber: cem.Jan 15 2018, 9:56 PM
cem added inline comments.
usr.sbin/kldxref/ef.c
516

why would ofs be -1? Other methods don't seem to check for this condition.

517

Why lseek+read instead of ef_read or pread?

527

inconsistent return style -- probably just remove parens

usr.sbin/kldxref/ef_obj.c
312

same as above (s.a.)

316

is this a tab?

emaste added inline comments.Jan 15 2018, 10:04 PM
usr.sbin/kldxref/ef.c
516

ef_read uses (Elf_Off)-1 to mean "no seek", this keeps the same interface

527

kldxref is already quite inconsistent; I count 47 return ( and 66 return [^(]. IMO it makes sense to follow style(9) when there's this much existing inconsistency.

cem added inline comments.Jan 15 2018, 10:10 PM
usr.sbin/kldxref/ef.c
527

That's an ok theory I can get behind, but this patch isn't internally consistent either. See the two returns directly above (0).

emaste added inline comments.Jan 16 2018, 2:48 AM
usr.sbin/kldxref/ef.c
527

Fair enough, the patch should at least be self-consistent.

mhorne063_gmail.com added inline comments.Jan 16 2018, 3:19 PM
usr.sbin/kldxref/ef.c
517

ef_read expects to read a certain length and returns an error if it comes up short, which is what we are working around here. On the other hand, pread seems slightly cleaner for this purpose so I will update it to use that.

mhorne063_gmail.com updated this revision to Diff 38036.Jan 16 2018, 3:21 PM
mhorne063_gmail.com marked 3 inline comments as done.
emaste added inline comments.Jan 16 2018, 3:23 PM
usr.sbin/kldxref/ef.c
517

Note that in the earlier patch we accepted (Elf_Off)-1 as "no seek", and that precluded using pread. But we don't need this for strings so Mitchell dropped it.

kib added a subscriber: kib.Jan 16 2018, 4:00 PM
kib added inline comments.
usr.sbin/kldxref/ef.c
104

I t is tempting to use the chance and switch to designated initializers.

506

No initialization in declarations.

Wny u_long ?

518

Why not return errno there ?

537

Again, why not return (errno) ?

mhorne063_gmail.com updated this revision to Diff 38040.Jan 16 2018, 4:48 PM

A lot of these comments address issues/inconsistencies already present in the source. I will create a subsequent cleanup patch to address these but follow what is there at the moment for this patch, since @emaste wants this merged to fix the lld build process.

mhorne063_gmail.com marked an inline comment as done.Jan 16 2018, 4:49 PM
kib accepted this revision.Jan 16 2018, 5:59 PM
This revision is now accepted and ready to land.Jan 16 2018, 5:59 PM
cem accepted this revision.Jan 16 2018, 5:59 PM
cem added inline comments.
usr.sbin/kldxref/ef.c
506

Matches existing style of file -- ok for now, IMO.

Closed by commit rS328052: kldxref: handle modules with md_cval at the end of allocated sections (authored by emaste). · Explain WhyJan 16 2018, 6:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptJan 16 2018, 6:20 PM
emaste mentioned this in rS328094: kldxref: improve style(9).Jan 17 2018, 8:00 PM

Revision Contents
Changeset List

PathSize
usr.sbin/
kldxref/
4 lines
30 lines
24 lines
3 lines

Diff 37990

View Options

usr.sbin/kldxref/ef.h

Show First 20 Lines • Show All 774 Lines • ▼ Show 20 Lines case F_READAHEAD:
*/ */
error = vn_lock(vp, LK_EXCLUSIVE); error = vn_lock(vp, LK_EXCLUSIVE);
if (error != 0) { if (error != 0) {
fdrop(fp, td); fdrop(fp, td);
break; break;
} }
if (arg >= 0) { if (arg >= 0) {
bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
fp->f_seqcount = (arg + bsize - 1) / bsize; arg = MIN(arg, INT_MAX - bsize + 1);
fp->f_seqcount = MIN(IO_SEQMAX,
(arg + bsize - 1) / bsize);
cemUnsubmitted
Not Done
Inline Actions

style(9): 4 space indent on trailing lines

cem: style(9): 4 space indent on trailing lines
atomic_set_int(&fp->f_flag, FRDAHEAD); atomic_set_int(&fp->f_flag, FRDAHEAD);
} else { } else {
atomic_clear_int(&fp->f_flag, FRDAHEAD); atomic_clear_int(&fp->f_flag, FRDAHEAD);
} }
VOP_UNLOCK(vp, 0); VOP_UNLOCK(vp, 0);
fdrop(fp, td); fdrop(fp, td);
break; break;
▲ Show 20 Lines • Show All 3,442 Lines • Show Last 20 Lines
View Options

usr.sbin/kldxref/ef.c

Show First 20 Lines • Show All 774 Lines • ▼ Show 20 Lines case F_READAHEAD:
*/ */
error = vn_lock(vp, LK_EXCLUSIVE); error = vn_lock(vp, LK_EXCLUSIVE);
if (error != 0) { if (error != 0) {
fdrop(fp, td); fdrop(fp, td);
break; break;
} }
if (arg >= 0) { if (arg >= 0) {
bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
fp->f_seqcount = (arg + bsize - 1) / bsize; arg = MIN(arg, INT_MAX - bsize + 1);
fp->f_seqcount = MIN(IO_SEQMAX,
(arg + bsize - 1) / bsize);
cemUnsubmitted
Not Done
Inline Actions

style(9): 4 space indent on trailing lines

cem: style(9): 4 space indent on trailing lines
atomic_set_int(&fp->f_flag, FRDAHEAD); atomic_set_int(&fp->f_flag, FRDAHEAD);
} else { } else {
atomic_clear_int(&fp->f_flag, FRDAHEAD); atomic_clear_int(&fp->f_flag, FRDAHEAD);
} }
VOP_UNLOCK(vp, 0); VOP_UNLOCK(vp, 0);
fdrop(fp, td); fdrop(fp, td);
break; break;
▲ Show 20 Lines • Show All 3,442 Lines • Show Last 20 Lines
View Options

usr.sbin/kldxref/ef_obj.c

Show First 20 Lines • Show All 774 Lines • ▼ Show 20 Lines case F_READAHEAD:
*/ */
error = vn_lock(vp, LK_EXCLUSIVE); error = vn_lock(vp, LK_EXCLUSIVE);
if (error != 0) { if (error != 0) {
fdrop(fp, td); fdrop(fp, td);
break; break;
} }
if (arg >= 0) { if (arg >= 0) {
bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
fp->f_seqcount = (arg + bsize - 1) / bsize; arg = MIN(arg, INT_MAX - bsize + 1);
fp->f_seqcount = MIN(IO_SEQMAX,
(arg + bsize - 1) / bsize);
cemUnsubmitted
Not Done
Inline Actions

style(9): 4 space indent on trailing lines

cem: style(9): 4 space indent on trailing lines
atomic_set_int(&fp->f_flag, FRDAHEAD); atomic_set_int(&fp->f_flag, FRDAHEAD);
} else { } else {
atomic_clear_int(&fp->f_flag, FRDAHEAD); atomic_clear_int(&fp->f_flag, FRDAHEAD);
} }
VOP_UNLOCK(vp, 0); VOP_UNLOCK(vp, 0);
fdrop(fp, td); fdrop(fp, td);
break; break;
▲ Show 20 Lines • Show All 3,442 Lines • Show Last 20 Lines
View Options

usr.sbin/kldxref/kldxref.c

Show First 20 Lines • Show All 774 Lines • ▼ Show 20 Lines case F_READAHEAD:
*/ */
error = vn_lock(vp, LK_EXCLUSIVE); error = vn_lock(vp, LK_EXCLUSIVE);
if (error != 0) { if (error != 0) {
fdrop(fp, td); fdrop(fp, td);
break; break;
} }
if (arg >= 0) { if (arg >= 0) {
bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
fp->f_seqcount = (arg + bsize - 1) / bsize; arg = MIN(arg, INT_MAX - bsize + 1);
fp->f_seqcount = MIN(IO_SEQMAX,
(arg + bsize - 1) / bsize);
cemUnsubmitted
Not Done
Inline Actions

style(9): 4 space indent on trailing lines

cem: style(9): 4 space indent on trailing lines
atomic_set_int(&fp->f_flag, FRDAHEAD); atomic_set_int(&fp->f_flag, FRDAHEAD);
} else { } else {
atomic_clear_int(&fp->f_flag, FRDAHEAD); atomic_clear_int(&fp->f_flag, FRDAHEAD);
} }
VOP_UNLOCK(vp, 0); VOP_UNLOCK(vp, 0);
fdrop(fp, td); fdrop(fp, td);
break; break;
▲ Show 20 Lines • Show All 3,442 Lines • Show Last 20 Lines