Page MenuHomeFreeBSD

Factor out checks for mounted root file system
ClosedPublic

Authored by hselasky on Wed, Nov 27, 3:26 PM.

Details

Summary

PR: 241639
MFC after: 1 week
Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Wed, Nov 27, 3:26 PM
kib edited the summary of this revision. (Show Details)Wed, Nov 27, 3:28 PM
hselasky added inline comments.Wed, Nov 27, 3:29 PM
sys/compat/linuxkpi/common/include/linux/kmod.h
46 ↗(On Diff #64938)

Should you check if p_fd != NULL ?

I wonder if this check should be factored out? It is not only the LinuxKPI which access these code paths too early, but USB drivers loading firmware .ko's can also do the same??

Index: sys/kern/kern_linker.c
===================================================================
--- sys/kern/kern_linker.c	(revision 355108)
+++ sys/kern/kern_linker.c	(working copy)
@@ -2067,6 +2067,7 @@
     struct linker_file *parent, const struct mod_depend *verinfo,
     struct linker_file **lfpp)
 {
+	struct proc *cp = curproc;
 	linker_file_t lfdep;
 	const char *filename;
 	char *pathname;
@@ -2079,13 +2080,13 @@
  		 */
 		KASSERT(verinfo == NULL, ("linker_load_module: verinfo"
 		    " is not NULL"));
-		if (rootvnode == NULL)
+		if (rootvnode == NULL || cp->p_fd == NULL || cp->p_fd->fd_rdir == NULL)
 			return (ENXIO);
 		pathname = linker_search_kld(kldname);
 	} else {
 		if (modlist_lookup2(modname, verinfo) != NULL)
 			return (EEXIST);
-		if (rootvnode == NULL)
+		if (rootvnode == NULL || cp->p_fd == NULL || cp->p_fd->fd_rdir == NULL)
 			return (ENXIO);
 		if (kldname != NULL)
 			pathname = strdup(kldname, M_LINKER);

Why not like this? Or remove the rootvnode == NULL and only test fd_rdir ?

--HPS

hselasky commandeered this revision.Wed, Nov 27, 3:56 PM
hselasky edited reviewers, added: kib; removed: hselasky.
kib added a comment.Wed, Nov 27, 3:59 PM
Index: sys/kern/kern_linker.c
===================================================================
--- sys/kern/kern_linker.c	(revision 355108)
+++ sys/kern/kern_linker.c	(working copy)
@@ -2067,6 +2067,7 @@
     struct linker_file *parent, const struct mod_depend *verinfo,
     struct linker_file **lfpp)
 {
+	struct proc *cp = curproc;
 	linker_file_t lfdep;
 	const char *filename;
 	char *pathname;
@@ -2079,13 +2080,13 @@
  		 */
 		KASSERT(verinfo == NULL, ("linker_load_module: verinfo"
 		    " is not NULL"));
-		if (rootvnode == NULL)
+		if (rootvnode == NULL || cp->p_fd == NULL || cp->p_fd->fd_rdir == NULL)
 			return (ENXIO);
 		pathname = linker_search_kld(kldname);
 	} else {
 		if (modlist_lookup2(modname, verinfo) != NULL)
 			return (EEXIST);
-		if (rootvnode == NULL)
+		if (rootvnode == NULL || cp->p_fd == NULL || cp->p_fd->fd_rdir == NULL)
 			return (ENXIO);
 		if (kldname != NULL)
 			pathname = strdup(kldname, M_LINKER);

Why not like this? Or remove the rootvnode == NULL and only test fd_rdir ?

Check for p_fd == NULL is really not needed, because we initalize it even for proc0 with valid file descriptor table.

I think that keeping rootvnode check is still needed/useful.

hselasky updated this revision to Diff 64940.Wed, Nov 27, 4:00 PM
hselasky retitled this revision from Avoid calling into kern_kldload() for kernel processes. to Factor out checks for mounted root file system.
hselasky edited the summary of this revision. (Show Details)
hselasky updated this revision to Diff 64941.Wed, Nov 27, 4:11 PM

Keep the rootvnode check in case the root file system gets unmounted.
Update comment.

kib added inline comments.Wed, Nov 27, 4:16 PM
sys/kern/kern_linker.c
2070 ↗(On Diff #64941)

No initialization in declaration. We usually name the single variable in function designated a process pointer 'p'.

2084 ↗(On Diff #64941)

p_fd == NULL check is not needed.

hselasky updated this revision to Diff 64943.Wed, Nov 27, 4:21 PM
hselasky marked 2 inline comments as done.

Handle comments from @kib

kib accepted this revision.Wed, Nov 27, 4:29 PM

In fact, I do not see much need in introducing 'p': you reference curproc only once in each branch, so caching the pointer does not save anything.

This revision is now accepted and ready to land.Wed, Nov 27, 4:29 PM
This revision was automatically updated to reflect the committed changes.