NetBSD Problem Report #35351
From yamt@mwd.biglobe.ne.jp Mon Jan 1 23:36:43 2007
Return-Path: <yamt@mwd.biglobe.ne.jp>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
by narn.NetBSD.org (Postfix) with ESMTP id BB7AE63B880
for <gnats-bugs@gnats.NetBSD.org>; Mon, 1 Jan 2007 23:36:43 +0000 (UTC)
Message-Id: <20070101233642.0BE0C11702@yamt.dyndns.org>
Date: Tue, 2 Jan 2007 08:36:42 +0900 (JST)
From: yamt@mwd.biglobe.ne.jp
Reply-To: yamt@mwd.biglobe.ne.jp
To: gnats-bugs@NetBSD.org
Subject: fileassoc lacks locking
X-Send-Pr-Version: 3.95
>Number: 35351
>Category: kern
>Synopsis: fileassoc lacks locking
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Mon Jan 01 23:40:00 +0000 2007
>Closed-Date:
>Last-Modified: Sun Jan 22 18:30:07 +0000 2012
>Originator: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
>Release: NetBSD 4.99.6
>Organization:
>Environment:
System: NetBSD bear.yamanet 4.99.6 NetBSD 4.99.6 (build.bear3) #3: Sat Dec 16 01:25:17 JST 2006 takashi@kaeru:/usr/home/takashi/work/kernel/build.bear3 i386
Architecture: i386
Machine: i386
>Description:
at least, hash table needs to have some kind of locking so that
duplicated entries are not added, etc.
>How-To-Repeat:
code inspection.
>Fix:
>Release-Note:
>Audit-Trail:
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: rmind@NetBSD.org, source-changes@NetBSD.org, core@netbsd.org,
gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 12:36:35 +0200
YAMAMOTO Takashi wrote:
> [ add gnats cc: ]
>
>> YAMAMOTO Takashi wrote:
>>>> the reason fileassoc lacked locking primitives is that I waited for
>>>> ad@ to commit and stablize newlock2. I *discussed* this pr with ad@
>>>> and yamt@, and, as a matter of fact, *prior* to newlock2, in a biglock
>>>> kernel, it is impossible to trigger the NULL deref.
>>> i've explained you that the PR is not (necessarily) about MP.
>>> did you understand it?
>>>
>>> (i'm not talking about this particular instance of NULL dereference.
>>> i'm not interested in analyzing how the known to be broken code can behave.
>>> i merely want to make sure that you know about the problem. if you already
>>> know, it's fine.)
>> I know.
>>
>> -e.
>
> ok, then you mean not to fix it for netbsd-4?
>
> YAMAMOTO Takashi
no, sorry. it's not a problem for netbsd-4.
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: rmind@NetBSD.org, source-changes@NetBSD.org, core@netbsd.org,
gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 7 Apr 2007 18:39:17 +0900 (JST)
> > ok, then you mean not to fix it for netbsd-4?
> >
> > YAMAMOTO Takashi
>
> no, sorry. it's not a problem for netbsd-4.
>
> -e.
i don't understand why it isn't a problem. can you explain?
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: rmind@NetBSD.org, source-changes@NetBSD.org, core@netbsd.org,
gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 12:42:25 +0200
YAMAMOTO Takashi wrote:
>>> ok, then you mean not to fix it for netbsd-4?
>>>
>>> YAMAMOTO Takashi
>> no, sorry. it's not a problem for netbsd-4.
>>
>> -e.
>
> i don't understand why it isn't a problem. can you explain?
>
> YAMAMOTO Takashi
I think I already did. can you tell me why it *is* a problem?
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: rmind@NetBSD.org, source-changes@NetBSD.org, core@netbsd.org,
gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 7 Apr 2007 18:56:49 +0900 (JST)
> YAMAMOTO Takashi wrote:
> >>> ok, then you mean not to fix it for netbsd-4?
> >>>
> >>> YAMAMOTO Takashi
> >> no, sorry. it's not a problem for netbsd-4.
> >>
> >> -e.
> >
> > i don't understand why it isn't a problem. can you explain?
> >
> > YAMAMOTO Takashi
>
> I think I already did.
sorry, where/when?
> can you tell me why it *is* a problem?
because, afaik, there is no difference wrt this problem between
netbsd-4 and current.
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: rmind@NetBSD.org, source-changes@NetBSD.org, core@netbsd.org,
gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 13:01:59 +0200
YAMAMOTO Takashi wrote:
>> YAMAMOTO Takashi wrote:
>>>>> ok, then you mean not to fix it for netbsd-4?
>>>>>
>>>>> YAMAMOTO Takashi
>>>> no, sorry. it's not a problem for netbsd-4.
>>>>
>>>> -e.
>>> i don't understand why it isn't a problem. can you explain?
>>>
>>> YAMAMOTO Takashi
>> I think I already did.
>
> sorry, where/when?
>
>> can you tell me why it *is* a problem?
>
> because, afaik, there is no difference wrt this problem between
> netbsd-4 and current.
>
> YAMAMOTO Takashi
are we talking about the pr itself or the null dereference commit?
whichever one it is, I'd like to see a scenario of where it can be
a problem.
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: rmind@NetBSD.org, tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 00:58:32 +0900 (JST)
[ remove core@ and source-changes@, add tech-kern@ ]
> YAMAMOTO Takashi wrote:
> >> YAMAMOTO Takashi wrote:
> >>>>> ok, then you mean not to fix it for netbsd-4?
> >>>>>
> >>>>> YAMAMOTO Takashi
> >>>> no, sorry. it's not a problem for netbsd-4.
> >>>>
> >>>> -e.
> >>> i don't understand why it isn't a problem. can you explain?
> >>>
> >>> YAMAMOTO Takashi
> >> I think I already did.
> >
> > sorry, where/when?
> >
> >> can you tell me why it *is* a problem?
> >
> > because, afaik, there is no difference wrt this problem between
> > netbsd-4 and current.
> >
> > YAMAMOTO Takashi
>
> are we talking about the pr itself or the null dereference commit?
i'm talking about the PR.
> whichever one it is, I'd like to see a scenario of where it can be
> a problem.
an example which i told you some time ago:
1. thread A calls fileassoc_file_add and sleeps in malloc.
2. thread B calls fileassoc_file_add and successfully allocates and inserts
a new entry.
3. thread A wakes up and inserts a duplicated entry.
do you mean it isn't a problem?
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: rmind@NetBSD.org, tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 19:05:07 +0200
YAMAMOTO Takashi wrote:
> an example which i told you some time ago:
> 1. thread A calls fileassoc_file_add and sleeps in malloc.
> 2. thread B calls fileassoc_file_add and successfully allocates and inserts
> a new entry.
> 3. thread A wakes up and inserts a duplicated entry.
can this scenario happen in netbsd-4 as a result of veriexecctl?
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: rmind@NetBSD.org, tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 01:10:41 +0900 (JST)
> YAMAMOTO Takashi wrote:
>
> > an example which i told you some time ago:
> > 1. thread A calls fileassoc_file_add and sleeps in malloc.
> > 2. thread B calls fileassoc_file_add and successfully allocates and inserts
> > a new entry.
> > 3. thread A wakes up and inserts a duplicated entry.
>
> can this scenario happen in netbsd-4 as a result of veriexecctl?
>
> -e.
i don't know.
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 19:11:15 +0200
YAMAMOTO Takashi wrote:
>> YAMAMOTO Takashi wrote:
>>
>>> an example which i told you some time ago:
>>> 1. thread A calls fileassoc_file_add and sleeps in malloc.
>>> 2. thread B calls fileassoc_file_add and successfully allocates and inserts
>>> a new entry.
>>> 3. thread A wakes up and inserts a duplicated entry.
>> can this scenario happen in netbsd-4 as a result of veriexecctl?
>>
>> -e.
>
> i don't know.
then what are we discussing, really? :)
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 01:34:20 +0900 (JST)
> YAMAMOTO Takashi wrote:
> >> YAMAMOTO Takashi wrote:
> >>
> >>> an example which i told you some time ago:
> >>> 1. thread A calls fileassoc_file_add and sleeps in malloc.
> >>> 2. thread B calls fileassoc_file_add and successfully allocates and inserts
> >>> a new entry.
> >>> 3. thread A wakes up and inserts a duplicated entry.
> >> can this scenario happen in netbsd-4 as a result of veriexecctl?
> >>
> >> -e.
> >
> > i don't know.
>
> then what are we discussing, really? :)
>
> -e.
the lack of locking in fileassoc. are you kidding?
as you know, veriexec is not only user of fileassoc.
do you mean you have investigated all of users and know fileassoc
doesn't need to have locking on netbsd-4? otherwise, why do you claim
it isn't a problem? to me, these users seem to work basically independently,
so i think nothing serializes users and thus fileassoc itself needs to
have locking internally.
am i missing something obvious? (i guess i am, given that you are
an author of all of these fileassoc users.)
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 19:48:25 +0200
YAMAMOTO Takashi wrote:
> the lack of locking in fileassoc. are you kidding?
>
>
> as you know, veriexec is not only user of fileassoc.
> do you mean you have investigated all of users and know fileassoc
> doesn't need to have locking on netbsd-4? otherwise, why do you claim
> it isn't a problem? to me, these users seem to work basically independently,
> so i think nothing serializes users and thus fileassoc itself needs to
> have locking internally.
> am i missing something obvious? (i guess i am, given that you are
> an author of all of these fileassoc users.)
(interesting use of the word "all". :)
besides veriexec there's only segvguard, for which the implementation
needs to change - it's unsafe even if locking was in place. I pointed
this out at least two times, and this is why it's disabled by default.
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 02:02:36 +0900 (JST)
> YAMAMOTO Takashi wrote:
>
> > the lack of locking in fileassoc. are you kidding?
> >
> >
> > as you know, veriexec is not only user of fileassoc.
> > do you mean you have investigated all of users and know fileassoc
> > doesn't need to have locking on netbsd-4? otherwise, why do you claim
> > it isn't a problem? to me, these users seem to work basically independently,
> > so i think nothing serializes users and thus fileassoc itself needs to
> > have locking internally.
> > am i missing something obvious? (i guess i am, given that you are
> > an author of all of these fileassoc users.)
>
> (interesting use of the word "all". :)
? sorry, i don't understand what you mean.
> besides veriexec there's only segvguard, for which the implementation
> needs to change - it's unsafe even if locking was in place. I pointed
> this out at least two times, and this is why it's disabled by default.
>
> -e.
is there a PR?
does veriexec serialize calls to fileassoc?
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 20:12:57 +0200
YAMAMOTO Takashi wrote:
> is there a PR?
no.
> does veriexec serialize calls to fileassoc?
given how veriexecctl works, I don't think it needs to.
(I'm getting rather tired of this thread. if you think fileassoc is
dangerous, fine, do whatever you like: disable it, disable veriexec,
remove whatever code you want from the tree, I *really* don't care.
I do think, however, that this will probably be a bigger disservice to
the netbsd project and user community than you think, and suggest you
try to take a look at what critical bugs netbsd-4 will ship with before
you're blowing this issue beyond proportion. the only user of fileassoc
in netbsd-4 which I'm willing to "back up" is veriexec, and given the
ioctl() calls, triggering fileassoc_add(), happen in a userland "loop"
in a single execution thread, I don't see a problem.)
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 02:43:51 +0900 (JST)
> YAMAMOTO Takashi wrote:
>
> > is there a PR?
>
> no.
can you consider to file one?
> (I'm getting rather tired of this thread.
so am i.
> the only user of fileassoc
> in netbsd-4 which I'm willing to "back up" is veriexec, and given the
> ioctl() calls, triggering fileassoc_add(), happen in a userland "loop"
> in a single execution thread, I don't see a problem.)
but there is nothing prevents multiple instances of the executables from
running, right?
anyway, i understood that you are considering that fileassoc (on netbsd-4)
is only for veriexec. it was not clear to me.
thanks for explanation.
YAMAMOTO Takashi
From: Elad Efrat <e@murder.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sat, 07 Apr 2007 20:50:19 +0200
YAMAMOTO Takashi wrote:
> but there is nothing prevents multiple instances of the executables from
> running, right?
/dev/veriexec is limited to one user at a time:
http://nxr.netbsd.org/source/xref/sys/dev/verified_exec.c#119
and that user must be root:
http://nxr.netbsd.org/source/xref/sys/dev/verified_exec.c#116
> anyway, i understood that you are considering that fileassoc (on netbsd-4)
> is only for veriexec. it was not clear to me.
for now, yes.
> thanks for explanation.
sure.
-e.
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: e@murder.org
Cc: tech-kern@netbsd.org, gnats-bugs@netbsd.org
Subject: Re: kern/35351 (Re: CVS commit: src/sys/kern)
Date: Sun, 8 Apr 2007 03:22:12 +0900 (JST)
> > but there is nothing prevents multiple instances of the executables from
> > running, right?
>
> /dev/veriexec is limited to one user at a time:
>
> http://nxr.netbsd.org/source/xref/sys/dev/verified_exec.c#119
>
> and that user must be root:
>
> http://nxr.netbsd.org/source/xref/sys/dev/verified_exec.c#116
ok, then it's somehow serialized. it's what i wanted to know. thanks.
YAMAMOTO Takashi
State-Changed-From-To: open->closed
State-Changed-By: dsl@NetBSD.org
State-Changed-When: Thu, 03 Dec 2009 22:38:01 +0000
State-Changed-Why:
Dialogue ends up saying that the originator agreed that there isn't a bug.
From: Elad Efrat <elad@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: dsl@NetBSD.org, yamt@mwd.biglobe.ne.jp
Subject: Re: kern/35351 (fileassoc lacks locking)
Date: Thu, 03 Dec 2009 20:22:56 -0500
dsl@NetBSD.org wrote:
> Synopsis: fileassoc lacks locking
>
> State-Changed-From-To: open->closed
> State-Changed-By: dsl@NetBSD.org
> State-Changed-When: Thu, 03 Dec 2009 22:38:01 +0000
> State-Changed-Why:
> Dialogue ends up saying that the originator agreed that there isn't a bug.
This is not the conclusion. The main issue discussed at the time
was whether this creates a problem for netbsd-4 or not. Fileassoc still
lacks locking (among other things) and if we want to make heavy use of
it these will need fixing.
Please re-open the PR. (Don't assign it to me though, I'm not interested
in automatic emails.)
Thanks,
-e.
State-Changed-From-To: closed->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 14 Dec 2009 01:30:15 +0000
State-Changed-Why:
Elad believes the PR should remain open.
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/35351: fileassoc lacks locking
Date: Sun, 22 Jan 2012 18:26:54 +0000
Resend to the correct address.
------
From: Elad Efrat <elad@NetBSD.org>
To: netbsd-bugs@netbsd.org
Subject: Fwd: PR/35351: fileassoc lacks locking
Date: Sun, 22 Jan 2012 11:39:29 -0500
Resend to a different address.
---------- Forwarded message ----------
From: Elad Efrat <elad@netbsd.org>
Date: Wed, Jan 18, 2012 at 12:48 PM
Subject: PR/35351: fileassoc lacks locking
To: gnats@netbsd.org
Attached is a diff that adds locking to fileassoc(9). It fixes two
panics that I can easily trigger through Veriexec.
A version of it (earlier? can't remember) was discussed here:
? ?http://mail-index.netbsd.org/tech-kern/2009/12/26/msg006703.html
I don't find the "it's not perfect" line of reasoning convincing, so
my recommendation is to check it in. It's filed so it doesn't get
lost.
Elad
Index: kern/kern_fileassoc.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_fileassoc.c,v
retrieving revision 1.34
diff -u -p -r1.34 kern_fileassoc.c
--- kern/kern_fileassoc.c 25 Dec 2009 20:07:18 -0000 1.34
+++ kern/kern_fileassoc.c 18 Jan 2012 17:21:55 -0000
@@ -58,9 +58,14 @@ struct fileassoc {
const char *assoc_name; /* Name. */
fileassoc_cleanup_cb_t assoc_cleanup_cb; /* Clear callback. */
specificdata_key_t assoc_key;
+
+ kmutex_t assoc_mtx;
+ kcondvar_t assoc_cv;
+ uint32_t assoc_refcnt;
};
static LIST_HEAD(, fileassoc) fileassoc_list;
+static kmutex_t fileassoc_list_lock;
/* An entry in the per-mount hash table. */
struct fileassoc_file {
@@ -68,6 +73,10 @@ struct fileassoc_file {
specificdata_reference faf_data; /* Assoc data. */
u_int faf_nassocs; /* # of assocs. */
LIST_ENTRY(fileassoc_file) faf_list; /* List pointer. */
+
+ kmutex_t faf_mtx;
+ kcondvar_t faf_cv;
+ uint32_t faf_refcnt;
};
LIST_HEAD(fileassoc_hash_entry, fileassoc_file);
@@ -78,15 +87,76 @@ struct fileassoc_table {
size_t tbl_nslots; /* Number of slots. */
size_t tbl_nused; /* # of used slots. */
specificdata_reference tbl_data;
+
+ kmutex_t tbl_mtx;
+ kcondvar_t tbl_cv;
+ uint32_t tbl_refcnt;
};
/*
* Hashing function: Takes a number modulus the mask to give back an
* index into the hash table.
*/
-#define FILEASSOC_HASH(tbl, handle) \
+#define FILEASSOC_HASH(mask, handle) \
(hash32_buf((handle), FHANDLE_SIZE(handle), HASH32_BUF_INIT) \
- & ((tbl)->tbl_mask))
+ & ((mask)))
+
+static void
+assoc_use(struct fileassoc *assoc)
+{
+
+ mutex_enter(&assoc->assoc_mtx);
+ assoc->assoc_refcnt++;
+ mutex_exit(&assoc->assoc_mtx);
+}
+
+static void
+assoc_unuse(struct fileassoc *assoc)
+{
+
+ mutex_enter(&assoc->assoc_mtx);
+ if (--assoc->assoc_refcnt == 0)
+ cv_broadcast(&assoc->assoc_cv);
+ mutex_exit(&assoc->assoc_mtx);
+}
+
+static void
+file_use(struct fileassoc_file *faf)
+{
+
+ mutex_enter(&faf->faf_mtx);
+ faf->faf_refcnt++;
+ mutex_exit(&faf->faf_mtx);
+}
+
+static void
+file_unuse(struct fileassoc_file *faf)
+{
+
+ mutex_enter(&faf->faf_mtx);
+ if (--faf->faf_refcnt == 0)
+ cv_broadcast(&faf->faf_cv);
+ mutex_exit(&faf->faf_mtx);
+}
+
+static void
+table_use(struct fileassoc_table *tbl)
+{
+
+ mutex_enter(&tbl->tbl_mtx);
+ tbl->tbl_refcnt++;
+ mutex_exit(&tbl->tbl_mtx);
+}
+
+static void
+table_unuse(struct fileassoc_table *tbl)
+{
+
+ mutex_enter(&tbl->tbl_mtx);
+ if (--tbl->tbl_refcnt == 0)
+ cv_broadcast(&tbl->tbl_cv);
+ mutex_exit(&tbl->tbl_mtx);
+}
static void *
file_getdata(struct fileassoc_file *faf, const struct fileassoc *assoc)
@@ -124,34 +194,70 @@ file_free(struct fileassoc_file *faf)
{
struct fileassoc *assoc;
- LIST_REMOVE(faf, faf_list);
+ KASSERT(faf->faf_refcnt == 0);
+ mutex_enter(&fileassoc_list_lock);
LIST_FOREACH(assoc, &fileassoc_list, assoc_list) {
+ assoc_use(assoc);
file_cleanup(faf, assoc);
+ assoc_unuse(assoc);
}
+ mutex_exit(&fileassoc_list_lock);
+
+ mutex_destroy(&faf->faf_mtx);
+ cv_destroy(&faf->faf_cv);
vfs_composefh_free(faf->faf_handle);
specificdata_fini(fileassoc_domain, &faf->faf_data);
+
kmem_free(faf, sizeof(*faf));
}
+/*
+ * Free a table.
+ *
+ * Expects the table to be detached and unattainable by anyone.
+ *
+ * Can be called in two cases:
+ * - fileassoc_table_delete(), called manually to delete a table,
+ * - specificdata(9), when a mount-point disappears.
+ */
static void
table_dtor(void *v)
{
struct fileassoc_table *tbl = v;
u_long i;
+ /* Wait for everyone to finish. */
+ mutex_enter(&tbl->tbl_mtx);
+ while (tbl->tbl_refcnt > 0)
+ cv_wait(&tbl->tbl_cv, &tbl->tbl_mtx);
+ mutex_exit(&tbl->tbl_mtx);
+
/* Remove all entries from the table and lists */
for (i = 0; i < tbl->tbl_nslots; i++) {
struct fileassoc_file *faf;
while ((faf = LIST_FIRST(&tbl->tbl_hash[i])) != NULL) {
+ /* Get exclusivity on this file. */
+ mutex_enter(&faf->faf_mtx);
+ while (faf->faf_refcnt > 0)
+ cv_wait(&faf->faf_cv, &faf->faf_mtx);
+
+ /* Remove it... */
+ LIST_REMOVE(faf, faf_list);
+
+ mutex_exit(&faf->faf_mtx);
+
file_free(faf);
}
}
- /* Remove hash table and sysctl node */
hashdone(tbl->tbl_hash, HASH_LIST, tbl->tbl_mask);
specificdata_fini(fileassoc_domain, &tbl->tbl_data);
+
+ mutex_destroy(&tbl->tbl_mtx);
+ cv_destroy(&tbl->tbl_cv);
+
kmem_free(tbl, sizeof(*tbl));
}
@@ -170,6 +276,8 @@ fileassoc_init(void)
}
fileassoc_domain = specificdata_domain_create();
+ mutex_init(&fileassoc_list_lock, MUTEX_DEFAULT, IPL_NONE);
+
return 0;
}
@@ -196,8 +304,13 @@ fileassoc_register(const char *name, fil
assoc->assoc_name = name;
assoc->assoc_cleanup_cb = cleanup_cb;
assoc->assoc_key = key;
+ mutex_init(&assoc->assoc_mtx, MUTEX_DEFAULT, IPL_NONE);
+ cv_init(&assoc->assoc_cv, "fa_assoc");
+ assoc->assoc_refcnt = 0;
+ mutex_enter(&fileassoc_list_lock);
LIST_INSERT_HEAD(&fileassoc_list, assoc, assoc_list);
+ mutex_exit(&fileassoc_list_lock);
*result = assoc;
@@ -211,8 +324,22 @@ int
fileassoc_deregister(fileassoc_t assoc)
{
+ mutex_enter(&fileassoc_list_lock);
+ mutex_enter(&assoc->assoc_mtx);
LIST_REMOVE(assoc, assoc_list);
+ mutex_exit(&fileassoc_list_lock);
+
+ /* Wait for everyone to finish playing with it... */
+ while (assoc->assoc_refcnt != 0)
+ cv_wait(&assoc->assoc_cv, &assoc->assoc_mtx);
+
specificdata_key_delete(fileassoc_domain, assoc->assoc_key);
+
+ mutex_exit(&assoc->assoc_mtx);
+
+ mutex_destroy(&assoc->assoc_mtx);
+ cv_destroy(&assoc->assoc_cv);
+
kmem_free(assoc, sizeof(*assoc));
return 0;
@@ -220,6 +347,7 @@ fileassoc_deregister(fileassoc_t assoc)
/*
* Get the hash table for the specified device.
+ *
*/
static struct fileassoc_table *
fileassoc_table_lookup(struct mount *mp)
@@ -230,6 +358,7 @@ fileassoc_table_lookup(struct mount *mp)
if (error) {
return NULL;
}
+
return mount_getspecific(mp, fileassoc_mountspecific_key);
}
@@ -253,26 +382,39 @@ fileassoc_file_lookup(struct vnode *vp,
return NULL;
}
+ table_use(tbl);
+
if (hint == NULL) {
error = vfs_composefh_alloc(vp, &th);
- if (error)
+ if (error) {
+ table_unuse(tbl);
return (NULL);
+ }
} else {
th = hint;
}
- indx = FILEASSOC_HASH(tbl, th);
+ indx = FILEASSOC_HASH(tbl->tbl_mask, th);
hash_entry = &(tbl->tbl_hash[indx]);
LIST_FOREACH(faf, hash_entry, faf_list) {
+ file_use(faf);
+
if (((FHANDLE_FILEID(faf->faf_handle)->fid_len ==
FHANDLE_FILEID(th)->fid_len)) &&
(memcmp(FHANDLE_FILEID(faf->faf_handle), FHANDLE_FILEID(th),
(FHANDLE_FILEID(th))->fid_len) == 0)) {
break;
}
+
+ file_unuse(faf);
}
+ if (faf != NULL)
+ file_unuse(faf);
+
+ table_unuse(tbl);
+
if (hint == NULL)
vfs_composefh_free(th);
@@ -286,35 +428,60 @@ void *
fileassoc_lookup(struct vnode *vp, fileassoc_t assoc)
{
struct fileassoc_file *faf;
+ void *data;
faf = fileassoc_file_lookup(vp, NULL);
if (faf == NULL)
return (NULL);
- return file_getdata(faf, assoc);
+ /* Prevent this file entry and assoc from being taken away. */
+ /* file_use(faf);
+ assoc_use(assoc); */
+
+ data = file_getdata(faf, assoc);
+
+ return data;
}
-static struct fileassoc_table *
+void
+fileassoc_release(struct vnode *vp, fileassoc_t assoc)
+{
+ struct fileassoc_file *faf;
+
+ faf = fileassoc_file_lookup(vp, NULL);
+ /*if (faf != NULL)
+ file_unuse(faf);
+
+ assoc_unuse(assoc);*/
+}
+
+/*
+ * Resize a fileassoc table.
+ *
+ * Expects tbl to be held exclusively by the caller.
+ */
+static int
fileassoc_table_resize(struct fileassoc_table *tbl)
{
- struct fileassoc_table *newtbl;
- u_long i;
+ struct fileassoc_hash_entry *new_hash;
+ size_t new_nslots;
+ u_long new_mask, i;
+
+ KASSERT(mutex_owned(&tbl->tbl_mtx));
+ KASSERT(tbl->tbl_refcnt == 1);
/*
- * Allocate a new table. Like the condition in fileassoc_file_add(),
+ * Allocate a new has table. Like the condition in fileassoc_file_add(),
* this is also temporary -- just double the number of slots.
*/
- newtbl = kmem_zalloc(sizeof(*newtbl), KM_SLEEP);
- newtbl->tbl_nslots = (tbl->tbl_nslots * 2);
- if (newtbl->tbl_nslots < tbl->tbl_nslots)
- newtbl->tbl_nslots = tbl->tbl_nslots;
- newtbl->tbl_hash = hashinit(newtbl->tbl_nslots, HASH_LIST,
- true, &newtbl->tbl_mask);
- newtbl->tbl_nused = 0;
- specificdata_init(fileassoc_domain, &newtbl->tbl_data);
-
- /* XXX we need to make sure nothing uses fileassoc here! */
+ new_nslots = (tbl->tbl_nslots * 2);
+ if (new_nslots < tbl->tbl_nslots)
+ new_nslots = tbl->tbl_nslots;
+ new_hash = hashinit(new_nslots, HASH_LIST, true, &new_mask);
+ /*
+ * Attach all entries to the new hash table.
+ */
for (i = 0; i < tbl->tbl_nslots; i++) {
struct fileassoc_file *faf;
@@ -322,31 +489,35 @@ fileassoc_table_resize(struct fileassoc_
struct fileassoc_hash_entry *hash_entry;
size_t indx;
+ /* Wait for others to finish with each file. */
+ mutex_enter(&faf->faf_mtx);
+ while (faf->faf_refcnt > 0)
+ cv_wait(&faf->faf_cv, &faf->faf_mtx);
+
LIST_REMOVE(faf, faf_list);
- indx = FILEASSOC_HASH(newtbl, faf->faf_handle);
- hash_entry = &(newtbl->tbl_hash[indx]);
+ indx = FILEASSOC_HASH(new_mask, faf->faf_handle);
+ hash_entry = &(new_hash[indx]);
LIST_INSERT_HEAD(hash_entry, faf, faf_list);
- newtbl->tbl_nused++;
+ mutex_exit(&faf->faf_mtx);
}
}
- if (tbl->tbl_nused != newtbl->tbl_nused)
- panic("fileassoc_table_resize: inconsistency detected! "
- "needed %zu entries, got %zu", tbl->tbl_nused,
- newtbl->tbl_nused);
-
+ /* Free the old hash table, and plug in the new one. */
hashdone(tbl->tbl_hash, HASH_LIST, tbl->tbl_mask);
- specificdata_fini(fileassoc_domain, &tbl->tbl_data);
- kmem_free(tbl, sizeof(*tbl));
+ tbl->tbl_hash = new_hash;
+ tbl->tbl_nslots = new_nslots;
+ tbl->tbl_mask = new_mask;
- return (newtbl);
+ return (0);
}
/*
* Create a new fileassoc table.
+ *
+ * The new table is returned with reference count set to 1.
*/
static struct fileassoc_table *
fileassoc_table_add(struct mount *mp)
@@ -365,6 +536,9 @@ fileassoc_table_add(struct mount *mp)
&tbl->tbl_mask);
tbl->tbl_nused = 0;
specificdata_init(fileassoc_domain, &tbl->tbl_data);
+ tbl->tbl_refcnt = 1;
+ mutex_init(&tbl->tbl_mtx, MUTEX_DEFAULT, IPL_NONE);
+ cv_init(&tbl->tbl_cv, "fa_tbl");
mount_setspecific(mp, fileassoc_mountspecific_key, tbl);
@@ -383,7 +557,10 @@ fileassoc_table_delete(struct mount *mp)
if (tbl == NULL)
return (EEXIST);
+ /* Detach it from the mount-point. */
mount_setspecific(mp, fileassoc_mountspecific_key, NULL);
+
+ /* Free it. */
table_dtor(tbl);
return (0);
@@ -403,18 +580,30 @@ fileassoc_table_run(struct mount *mp, fi
if (tbl == NULL)
return (EEXIST);
+ table_use(tbl);
+
+ assoc_use(assoc);
+
for (i = 0; i < tbl->tbl_nslots; i++) {
struct fileassoc_file *faf;
LIST_FOREACH(faf, &tbl->tbl_hash[i], faf_list) {
void *data;
+ file_use(faf);
+
data = file_getdata(faf, assoc);
if (data != NULL)
cb(data, cookie);
+
+ file_unuse(faf);
}
}
+ assoc_unuse(assoc);
+
+ table_unuse(tbl);
+
return (0);
}
@@ -431,20 +620,36 @@ fileassoc_table_clear(struct mount *mp,
if (tbl == NULL)
return (EEXIST);
+ table_use(tbl);
+
+ assoc_use(assoc);
+
for (i = 0; i < tbl->tbl_nslots; i++) {
struct fileassoc_file *faf;
LIST_FOREACH(faf, &tbl->tbl_hash[i], faf_list) {
+ mutex_enter(&faf->faf_mtx);
+ while (faf->faf_refcnt > 0)
+ cv_wait(&faf->faf_cv, &faf->faf_mtx);
+
file_cleanup(faf, assoc);
file_setdata(faf, assoc, NULL);
+
+ mutex_exit(&faf->faf_mtx);
}
}
+ assoc_unuse(assoc);
+
+ table_unuse(tbl);
+
return (0);
}
/*
* Add a file entry to a table.
+ *
+ * XXX: Set reference count to 1 here too?
*/
static struct fileassoc_file *
fileassoc_file_add(struct vnode *vp, fhandle_t *hint)
@@ -473,16 +678,24 @@ fileassoc_file_add(struct vnode *vp, fha
tbl = fileassoc_table_lookup(vp->v_mount);
if (tbl == NULL) {
+ /* This will "keep" a reference for us. */
tbl = fileassoc_table_add(vp->v_mount);
+ } else {
+ /* Keep a reference to the table. */
+ table_use(tbl);
}
- indx = FILEASSOC_HASH(tbl, th);
- hash_entry = &(tbl->tbl_hash[indx]);
-
faf = kmem_zalloc(sizeof(*faf), KM_SLEEP);
faf->faf_handle = th;
specificdata_init(fileassoc_domain, &faf->faf_data);
- LIST_INSERT_HEAD(hash_entry, faf, faf_list);
+ faf->faf_refcnt = 0;
+ mutex_init(&faf->faf_mtx, MUTEX_DEFAULT, IPL_NONE);
+ cv_init(&faf->faf_cv, "fa_faf");
+
+ /* We need the table exclusively. */
+ mutex_enter(&tbl->tbl_mtx);
+ while (tbl->tbl_refcnt > 1)
+ cv_wait(&tbl->tbl_cv, &tbl->tbl_mtx);
/*
* This decides when we need to resize the table. For now,
@@ -490,14 +703,19 @@ fileassoc_file_add(struct vnode *vp, fha
* has. That's not really true unless of course we had zero
* collisions. Think positive! :)
*/
- if (++(tbl->tbl_nused) == tbl->tbl_nslots) {
- struct fileassoc_table *newtbl;
-
- newtbl = fileassoc_table_resize(tbl);
- mount_setspecific(vp->v_mount, fileassoc_mountspecific_key,
- newtbl);
+ if (((tbl->tbl_nused) + 1) == tbl->tbl_nslots) {
+ fileassoc_table_resize(tbl);
}
+ indx = FILEASSOC_HASH(tbl->tbl_mask, th);
+ hash_entry = &(tbl->tbl_hash[indx]);
+ LIST_INSERT_HEAD(hash_entry, faf, faf_list);
+ ++(tbl->tbl_nused);
+
+ mutex_exit(&tbl->tbl_mtx);
+
+ table_unuse(tbl);
+
return (faf);
}
@@ -518,10 +736,23 @@ fileassoc_file_delete(struct vnode *vp)
return (ENOENT);
}
+ /* Wait for everyone to finish... */
+ mutex_enter(&faf->faf_mtx);
+ while (faf->faf_refcnt > 0)
+ cv_wait(&faf->faf_cv, &faf->faf_mtx);
+
+ /* Remove it... */
+ LIST_REMOVE(faf, faf_list);
+
+ mutex_exit(&faf->faf_mtx);
+
file_free(faf);
tbl = fileassoc_table_lookup(vp->v_mount);
+
+ mutex_enter(&tbl->tbl_mtx);
--(tbl->tbl_nused); /* XXX gc? */
+ mutex_exit(&tbl->tbl_mtx);
KERNEL_UNLOCK_ONE(NULL);
@@ -544,14 +775,25 @@ fileassoc_add(struct vnode *vp, fileasso
return (ENOTDIR);
}
+ mutex_enter(&faf->faf_mtx);
+
+ assoc_use(assoc);
+
olddata = file_getdata(faf, assoc);
- if (olddata != NULL)
+ if (olddata != NULL) {
+ assoc_unuse(assoc);
+ mutex_exit(&faf->faf_mtx);
return (EEXIST);
+ }
file_setdata(faf, assoc, data);
faf->faf_nassocs++;
+ assoc_unuse(assoc);
+
+ mutex_exit(&faf->faf_mtx);
+
return (0);
}
@@ -567,10 +809,20 @@ fileassoc_clear(struct vnode *vp, fileas
if (faf == NULL)
return (ENOENT);
+ file_use(faf);
+
+ assoc_use(assoc);
+
file_cleanup(faf, assoc);
file_setdata(faf, assoc, NULL);
+ assoc_unuse(assoc);
+
+ mutex_enter(&faf->faf_mtx);
--(faf->faf_nassocs); /* XXX gc? */
+ mutex_exit(&faf->faf_mtx);
+
+ file_unuse(faf);
return (0);
}
Index: sys/fileassoc.h
===================================================================
RCS file: /cvsroot/src/sys/sys/fileassoc.h,v
retrieving revision 1.11
diff -u -p -r1.11 fileassoc.h
--- sys/fileassoc.h 15 May 2007 19:47:44 -0000 1.11
+++ sys/fileassoc.h 18 Jan 2012 17:21:55 -0000
@@ -45,6 +45,7 @@ int fileassoc_table_clear(struct mount *
int fileassoc_file_delete(struct vnode *);
int fileassoc_add(struct vnode *, fileassoc_t, void *);
int fileassoc_clear(struct vnode *, fileassoc_t);
+void fileassoc_release(struct vnode *, fileassoc_t);
int fileassoc_table_run(struct mount *, fileassoc_t, fileassoc_cb_t, void *);
#endif /* !_SYS_FILEASSOC_H_ */
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/35351: fileassoc lacks locking
Date: Sun, 22 Jan 2012 18:28:25 +0000
forward two more mails to gnats
------
From: David Laight <david@l8s.co.uk>
To: Elad Efrat <elad@NetBSD.org>
Cc: netbsd-bugs@netbsd.org
Subject: Re: Fwd: PR/35351: fileassoc lacks locking
Date: Sun, 22 Jan 2012 17:35:43 +0000
Mail-Followup-To: Elad Efrat <elad@NetBSD.org>, netbsd-bugs@netbsd.org
On Sun, Jan 22, 2012 at 11:39:29AM -0500, Elad Efrat wrote:
> Resend to a different address.
>
> ---------- Forwarded message ----------
> From: Elad Efrat <elad@netbsd.org>
> Date: Wed, Jan 18, 2012 at 12:48 PM
> Subject: PR/35351: fileassoc lacks locking
> To: gnats@netbsd.org
>
>
> Attached is a diff that adds locking to fileassoc(9). It fixes two
> panics that I can easily trigger through Veriexec.
A quick scan makes me think it has too many mutex and condvar.
David
--
David Laight: david@l8s.co.uk
From: Matthew Mondor <mm_lists@pulsar-zone.net>
To: netbsd-bugs@netbsd.org
Subject: Re: PR/35351: fileassoc lacks locking
Date: Sun, 22 Jan 2012 12:38:47 -0500
On Sun, 22 Jan 2012 11:39:29 -0500
Elad Efrat <elad@NetBSD.org> wrote:
> Attached is a diff that adds locking to fileassoc(9). It fixes two
> panics that I can easily trigger through Veriexec.
>
> A version of it (earlier? can't remember) was discussed here:
>
> ? ?http://mail-index.netbsd.org/tech-kern/2009/12/26/msg006703.html
>
> I don't find the "it's not perfect" line of reasoning convincing, so
> my recommendation is to check it in. It's filed so it doesn't get
> lost.
The only part that's not totally clear to me (from the diff alone) is
when a hash table is rehashed, where/when the old table gets freed?
+ /* Free the old hash table, and plug in the new one. */
hashdone(tbl->tbl_hash, HASH_LIST, tbl->tbl_mask);
- specificdata_fini(fileassoc_domain, &tbl->tbl_data);
- kmem_free(tbl, sizeof(*tbl));
+ tbl->tbl_hash = new_hash;
+ tbl->tbl_nslots = new_nslots;
+ tbl->tbl_mask = new_mask;
Of course I may have overlooked something too.
Thanks,
--
Matt
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home