lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 May 2008 09:40:13 +0200 From: Nadia Derbey <Nadia.Derbey@...l.net> To: Tim Pepper <lnxninja@...ux.vnet.ibm.com> Cc: manfred@...orfullife.com, paulmck@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org, efault@....de, akpm@...ux-foundation.org Subject: Re: [PATCH 7/9] Make idr_remove rcu-safe Tim Pepper wrote: > On Wed 07 May at 13:36:00 +0200 Nadia.Derbey@...l.net said: > >>[PATCH 07/09] >> >>This patch introduces the free_layer() routine: it is the one that actually >>frees memory after a grace period has elapsed. >> >>Signed-off-by: Nadia Derbey <Nadia.Derbey@...l.net> >> >>--- >> lib/idr.c | 59 ++++++++++++++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 44 insertions(+), 15 deletions(-) >> >>Index: linux-2.6.25-mm1/lib/idr.c >>=================================================================== >>--- linux-2.6.25-mm1.orig/lib/idr.c 2008-05-06 18:06:43.000000000 +0200 >>+++ linux-2.6.25-mm1/lib/idr.c 2008-05-07 09:07:31.000000000 +0200 >>@@ -424,15 +455,13 @@ void idr_remove_all(struct idr *idp) >> >> id += 1 << n; >> while (n < fls(id)) { >>- if (p) { >>- memset(p, 0, sizeof *p); >>- move_to_free_list(idp, p); >>- } >>+ if (p) >>+ free_layer(p); >> n += IDR_BITS; >> p = *--paa; >> } >> } >>- idp->top = NULL; >>+ rcu_assign_pointer(idp->top, NULL); >> idp->layers = 0; >> } >> EXPORT_SYMBOL(idr_remove_all); > > > Does idr_remove_all() need an rcu_dereference() in the loop preceeding the > above, where it does: > > while (n > IDR_BITS && p) { > n -= IDR_BITS; > *paa++ = p; > ----> p = p->ary[(id >> n) & IDR_MASK]; > } I assumed here that idr_remove_all() was called in the "typical cleanup sequence" mentioned in the comment describing the routine. And actually, that's how it is called in drivers/char/drm. > > idr_replace() also has that loop without rcu_derefernce, but I _think_ > I see why that one should be ok. At least there the comment is clear > that locking at a higher level should be happening. I didn't use rcu_dereference here since the caller should anyway serialize with other writers. So the tree should remain unchanged during the replace operation. > And then > idr_remove_all() is almost unused and it looks like it is only in > serialized places. > > Otherwise, thanks for redoing...This patch set was much easier to digest > and looks reasonable to me. > > I've been having some machine issues, but hope to give this patch set a run > still on a 128way machine and mainline to provide some additional > datapoints. > That would be kind, indeed (hope I didn't break anything). Regards, Nadia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists