Fix detection of kernel memory overlap
ClosedPublic
Actions

Authored by leandro.lupori_gmail.com on Apr 17 2018, 7:47 PM.

Details

Reviewers

jhibbits
nwhitehorn

Commits

rS332788: Fix detection of memory overlap with the kernel in the case where a memory

Summary

Function moea64_early_bootstrap was not detecting overlaps
between available and kernel memory regions when the
available region was entirely contained in kernel region.

This would result in kernel memory being considered as
available for other purposes, corrupting kernel memory
usage.

The fix consists in adding an additional check and exclude
memory regions inside [kernelstart, kernelend] range.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16200
Build 16152: arc lint + arc unit

Event Timeline

leandro.lupori_gmail.com created this revision.Apr 17 2018, 7:47 PM

Herald added a subscriber: jhibbits. · View Herald TranscriptApr 17 2018, 7:47 PM

Harbormaster completed remote builds in B16197: Diff 41592.Apr 17 2018, 7:47 PM

leandro.lupori_gmail.com added reviewers: jhibbits, nwhitehorn.Apr 17 2018, 7:50 PM

jhibbits added inline comments.Apr 17 2018, 8:10 PM

sys/powerpc/aim/mmu_oea64.c
705	Style nit: return values should be wrapped in ()'s (yes, we violate that in a few spots here, but it's style). Won't penalize for it, though.
788	Do you need the cast here? I think it's redundant, but may be wrong.

leandro.lupori_gmail.com added inline comments.Apr 17 2018, 9:05 PM

sys/powerpc/aim/mmu_oea64.c
705	Ok.
788	Without the cast it's harder to be sure that it will be correct for every platform. If phys_avail type will always be uint64_t [] in this code, than it should be safe to use either ~0UL or ~0ULL. But if it may be uint32_t, than using 0U and (possibly) widening it to vm_paddr_t before negating it would be better. ~0ULL should also work for uint32_t, as ~0ULL will probably be truncated to 32 bits in this case. But the compiler may produce a warning about narrowing the value. What do you think?

jhibbits added inline comments.Apr 17 2018, 9:11 PM

sys/powerpc/aim/mmu_oea64.c
788	~0 should suffice. We use that in other places to represent all-one values, such as in bus_alloc_resource_any. C rules will automagically return the correct value.

Addressing review's comments

Harbormaster completed remote builds in B16200: Diff 41597.Apr 17 2018, 9:20 PM

leandro.lupori_gmail.com marked 5 inline comments as done.Apr 17 2018, 9:21 PM

Great work!

This revision is now accepted and ready to land.Apr 18 2018, 3:20 PM

Thanks @jhibbits! Can you commit the change for me?

Booted r331763 with this change and it fixed the problem for me.

Closed by commit rS332788: Fix detection of memory overlap with the kernel in the case where a memory (authored by nwhitehorn). · Explain WhyApr 19 2018, 6:35 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptApr 19 2018, 6:35 PM

nwhitehorn mentioned this in rS332788: Fix detection of memory overlap with the kernel in the case where a memory.Apr 19 2018, 6:35 PM

Revision Contents
Changeset List

Path

Size

sys/

powerpc/

aim/

mmu_oea64.c

33 lines

Diff 41597

View Options

sys/powerpc/aim/mmu_oea64.c

Show First 20 Lines • Show All 116 Lines • ▼ Show 20 Lines
* Number of vnodes in existence. Increased whenever getnewvnode()		* Number of vnodes in existence. Increased whenever getnewvnode()
* allocates a new vnode, decreased in vdropl() for VI_DOOMED vnode.		* allocates a new vnode, decreased in vdropl() for VI_DOOMED vnode.
*/		*/
static unsigned long numvnodes;		static unsigned long numvnodes;

SYSCTL_ULONG(_vfs, OID_AUTO, numvnodes, CTLFLAG_RD, &numvnodes, 0,		SYSCTL_ULONG(_vfs, OID_AUTO, numvnodes, CTLFLAG_RD, &numvnodes, 0,
"Number of vnodes in existence");		"Number of vnodes in existence");

		static u_long getnewvnode_calls;
		SYSCTL_ULONG(_vfs, OID_AUTO, getnewvnode_calls, CTLFLAG_RD, &getnewvnode_calls,
		kibUnsubmitted Not Done Inline Actions Might be , vnodes_created ? kib: Might be , vnodes_created ?
		jhbAuthorUnsubmitted Not Done Inline Actions Ok. jhb: Ok.
		0, "Number of successful calls to getnewvnode");

/*		/*
* Conversion tables for conversion from vnode types to inode formats		* Conversion tables for conversion from vnode types to inode formats
* and back.		* and back.
*/		*/
enum vtype iftovt_tab[16] = {		enum vtype iftovt_tab[16] = {
VNON, VFIFO, VCHR, VNON, VDIR, VNON, VBLK, VNON,		VNON, VFIFO, VCHR, VNON, VDIR, VNON, VBLK, VNON,
VREG, VNON, VLNK, VNON, VSOCK, VNON, VNON, VBAD,		VREG, VNON, VLNK, VNON, VSOCK, VNON, VNON, VBAD,
};		};
Show All 18 Lines
static u_long freevnodes;		static u_long freevnodes;
SYSCTL_ULONG(_vfs, OID_AUTO, freevnodes, CTLFLAG_RD, &freevnodes, 0,		SYSCTL_ULONG(_vfs, OID_AUTO, freevnodes, CTLFLAG_RD, &freevnodes, 0,
"Number of vnodes in the free list");		"Number of vnodes in the free list");

static int vlru_allow_cache_src;		static int vlru_allow_cache_src;
SYSCTL_INT(_vfs, OID_AUTO, vlru_allow_cache_src, CTLFLAG_RW,		SYSCTL_INT(_vfs, OID_AUTO, vlru_allow_cache_src, CTLFLAG_RW,
&vlru_allow_cache_src, 0, "Allow vlru to reclaim source vnode");		&vlru_allow_cache_src, 0, "Allow vlru to reclaim source vnode");

		static u_long recycles_count;
		SYSCTL_ULONG(_vfs, OID_AUTO, recycles, CTLFLAG_RD, &recycles_count, 0,
		kibUnsubmitted Not Done Inline Actions Is this mib RW on purpose ? kib: Is this mib RW on purpose ?
		jhbAuthorUnsubmitted Not Done Inline Actions Eh, hmm. Some stat nodes are RW to allow them to be zero'd, but it looks like most of the VFS ones are read-only (though reassignbufcalls is RW). I'll switch it to read-only. jhb: Eh, hmm. Some stat nodes are RW to allow them to be zero'd, but it looks like most of the VFS…
		"Number of vnodes recycled to avoid exceding kern.maxvnodes");

/*		/*
* Various variables used for debugging the new implementation of		* Various variables used for debugging the new implementation of
* reassignbuf().		* reassignbuf().
* XXX these are probably of (very) limited utility now.		* XXX these are probably of (very) limited utility now.
*/		*/
static int reassignbufcalls;		static int reassignbufcalls;
SYSCTL_INT(_vfs, OID_AUTO, reassignbufcalls, CTLFLAG_RW, &reassignbufcalls, 0,		SYSCTL_INT(_vfs, OID_AUTO, reassignbufcalls, CTLFLAG_RW, &reassignbufcalls, 0,
"Number of calls to reassignbuf");		"Number of calls to reassignbuf");
▲ Show 20 Lines • Show All 616 Lines • ▼ Show 20 Lines	if (vp->v_usecount \|\|
(vp->v_object != NULL &&		(vp->v_object != NULL &&
vp->v_object->resident_page_count > trigger)) {		vp->v_object->resident_page_count > trigger)) {
VOP_UNLOCK(vp, LK_INTERLOCK);		VOP_UNLOCK(vp, LK_INTERLOCK);
vdrop(vp);		vdrop(vp);
goto next_iter_mntunlocked;		goto next_iter_mntunlocked;
}		}
KASSERT((vp->v_iflag & VI_DOOMED) == 0,		KASSERT((vp->v_iflag & VI_DOOMED) == 0,
("VI_DOOMED unexpectedly detected in vlrureclaim()"));		("VI_DOOMED unexpectedly detected in vlrureclaim()"));
		atomic_add_long(&recycles_count, 1);
vgonel(vp);		vgonel(vp);
VOP_UNLOCK(vp, 0);		VOP_UNLOCK(vp, 0);
vdropl(vp);		vdropl(vp);
done++;		done++;
next_iter_mntunlocked:		next_iter_mntunlocked:
if (!should_yield())		if (!should_yield())
goto relock_mnt;		goto relock_mnt;
goto yield;		goto yield;
▲ Show 20 Lines • Show All 184 Lines • ▼ Show 20 Lines	vtryrecycle(struct vnode *vp)
if (vp->v_usecount) {		if (vp->v_usecount) {
VOP_UNLOCK(vp, LK_INTERLOCK);		VOP_UNLOCK(vp, LK_INTERLOCK);
vn_finished_write(vnmp);		vn_finished_write(vnmp);
CTR2(KTR_VFS,		CTR2(KTR_VFS,
"%s: impossible to recycle, %p is already referenced",		"%s: impossible to recycle, %p is already referenced",
__func__, vp);		__func__, vp);
return (EBUSY);		return (EBUSY);
}		}
if ((vp->v_iflag & VI_DOOMED) == 0)		if ((vp->v_iflag & VI_DOOMED) == 0) {
		kibUnsubmitted Not Done Inline Actions I think we must only count when vgonel() call is performed, i.e. under the if() branch. kib: I think we must only count when vgonel() call is performed, i.e. under the if() branch.
		jhbAuthorUnsubmitted Not Done Inline Actions Agreed, I almost made that change but talked myself out of it. jhb: Agreed, I almost made that change but talked myself out of it.
		atomic_add_long(&recycles_count, 1);
vgonel(vp);		vgonel(vp);
		}
VOP_UNLOCK(vp, LK_INTERLOCK);		VOP_UNLOCK(vp, LK_INTERLOCK);
vn_finished_write(vnmp);		vn_finished_write(vnmp);
return (0);		return (0);
}		}

/*		/*
* Wait for available vnodes.		* Wait for available vnodes.
*/		*/
▲ Show 20 Lines • Show All 63 Lines • ▼ Show 20 Lines
getnewvnode(const char tag, struct mount mp, struct vop_vector *vops,		getnewvnode(const char tag, struct mount mp, struct vop_vector *vops,
struct vnode **vpp)		struct vnode **vpp)
{		{
struct vnode *vp;		struct vnode *vp;
struct bufobj *bo;		struct bufobj *bo;
struct thread *td;		struct thread *td;
int error;		int error;

CTR3(KTR_VFS, "%s: mp %p with tag %s", __func__, mp, tag);		CTR3(KTR_VFS, "%s: mp %p with tag %s", __func__, mp, tag);
		kibUnsubmitted Not Done Inline Actions Althought this is currently not enabled, getnewvnode() was designed to allow fail. Please move the increment after the alloc label, so that return (error) statement in the #if 0-block is still 'honored'. kib: Althought this is currently not enabled, getnewvnode() was designed to allow fail. Please move…
		jhbAuthorUnsubmitted Not Done Inline Actions Done. I've updated the counter's description to note that it only counts calls that succeed as well. Not sure if the name should also be changed from 'getnewvnode_calls' to 'getnewvnode_<mumble>' as well? (Can't think of a decent <mumble> off the top of my head) Maybe 'vnodes_allocated'? jhb: Done. I've updated the counter's description to note that it only counts calls that succeed as…
vp = NULL;		vp = NULL;
td = curthread;		td = curthread;
if (td->td_vp_reserv > 0) {		if (td->td_vp_reserv > 0) {
td->td_vp_reserv -= 1;		td->td_vp_reserv -= 1;
goto alloc;		goto alloc;
}		}
mtx_lock(&vnode_free_list_mtx);		mtx_lock(&vnode_free_list_mtx);
/*		/*
* Lend our context to reclaim vnodes if they've exceeded the max.		* Lend our context to reclaim vnodes if they've exceeded the max.
*/		*/
if (freevnodes > wantfreevnodes)		if (freevnodes > wantfreevnodes)
vnlru_free(1);		vnlru_free(1);
error = getnewvnode_wait(mp != NULL && (mp->mnt_kern_flag &		error = getnewvnode_wait(mp != NULL && (mp->mnt_kern_flag &
MNTK_SUSPEND));		MNTK_SUSPEND));
#if 0 /* XXX Not all VFS_VGET/ffs_vget callers check returns. */		#if 0 /* XXX Not all VFS_VGET/ffs_vget callers check returns. */
if (error != 0) {		if (error != 0) {
mtx_unlock(&vnode_free_list_mtx);		mtx_unlock(&vnode_free_list_mtx);
return (error);		return (error);
}		}
#endif		#endif
atomic_add_long(&numvnodes, 1);		atomic_add_long(&numvnodes, 1);
mtx_unlock(&vnode_free_list_mtx);		mtx_unlock(&vnode_free_list_mtx);
alloc:		alloc:
		atomic_add_long(&getnewvnode_calls, 1);
vp = (struct vnode *) uma_zalloc(vnode_zone, M_WAITOK\|M_ZERO);		vp = (struct vnode *) uma_zalloc(vnode_zone, M_WAITOK\|M_ZERO);
/*		/*
* Setup locks.		* Setup locks.
*/		*/
vp->v_vnlock = &vp->v_lock;		vp->v_vnlock = &vp->v_lock;
mtx_init(&vp->v_interlock, "vnode interlock", NULL, MTX_DEF);		mtx_init(&vp->v_interlock, "vnode interlock", NULL, MTX_DEF);
/*		/*
* By default, don't allow shared locks unless filesystems		* By default, don't allow shared locks unless filesystems
▲ Show 20 Lines • Show All 3,775 Lines • Show Last 20 Lines

Fix detection of kernel memory overlapClosedPublicActions