From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756242AbdJJNoc (ORCPT ); Tue, 10 Oct 2017 09:44:32 -0400 Received: from shelob.surriel.com ([96.67.55.147]:36474 "EHLO shelob.surriel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755919AbdJJNob (ORCPT ); Tue, 10 Oct 2017 09:44:31 -0400 Message-ID: <1507643056.21121.175.camel@surriel.com> Subject: Re: [PATCH v4 1/2] pid: Replace pid bitmap implementation with IDR API From: Rik van Riel To: Oleg Nesterov , Andrew Morton Cc: Gargi Sharma , linux-kernel@vger.kernel.org, julia.lawall@lip6.fr, mingo@kernel.org, pasha.tatashin@oracle.com, ktkhai@virtuozzo.com, ebiederm@xmission.com, hch@infradead.org Date: Tue, 10 Oct 2017 09:44:16 -0400 In-Reply-To: <20171010115034.GA28545@redhat.com> References: <1507583624-22146-1-git-send-email-gs051095@gmail.com> <1507583624-22146-2-git-send-email-gs051095@gmail.com> <20171009161737.ea8c62441cc12dfd909ee0b2@linux-foundation.org> <20171010115034.GA28545@redhat.com> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-Oq6r2pVxhtvyqeDjhZ0x" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-Oq6r2pVxhtvyqeDjhZ0x Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2017-10-10 at 13:50 +0200, Oleg Nesterov wrote: > On 10/09, Andrew Morton wrote: > >=20 > > > @@ -240,17 +230,11 @@ void zap_pid_ns_processes(struct > > > pid_namespace *pid_ns) > > > =C2=A0 =C2=A0* > > > =C2=A0 =C2=A0*/ > > > =C2=A0 read_lock(&tasklist_lock); > > > - nr =3D next_pidmap(pid_ns, 1); > > > - while (nr > 0) { > > > - rcu_read_lock(); > > > - > > > - task =3D pid_task(find_vpid(nr), PIDTYPE_PID); > > > + nr =3D 2; > > > + idr_for_each_entry_continue(&pid_ns->idr, pid, nr) { > > > + task =3D pid_task(pid, PIDTYPE_PID); > > > =C2=A0 if (task && !__fatal_signal_pending(task)) > > > =C2=A0 send_sig_info(SIGKILL, SEND_SIG_FORCED, > > > task); > > > - > > > - rcu_read_unlock(); > > > - > > > - nr =3D next_pidmap(pid_ns, nr); > > > =C2=A0 } > > > =C2=A0 read_unlock(&tasklist_lock); > >=20 > > Especially here.=C2=A0=C2=A0I don't think pidmap_lock is held.=C2=A0=C2= =A0Is that IDR > > iteration safe? >=20 > Yes, this doesn't look right, we need rcu_read_lock() or pidmap_lock. >=20 > And, we also need rcu_read_lock() for another reason, to protect > "struct pid". I think rcu_read_lock alone should do the trick, for both. The IDR code specifically says that lookups are safe under just the rcu_read_lock, and that only insertions and deletions need a separate lock for synchronization. Good catch. --=20 All Rights Reversed. --=-Oq6r2pVxhtvyqeDjhZ0x Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJZ3M6wAAoJEM553pKExN6DLkIH/1pm2LBm4NBQg8Dr8Q33UL7N 67kUA21XuTRqzCIEA1IJrW2LSpheo3mtihDGV5eVbNLg2s4PTbCSO4eLsBq9esUH ZJFeRMBn9jB64DL5mplJq8qdqyf131LcJAEzopvYxzifFcWsiuqcGAp/BgVizCtu yrES7OJeoIXA0G/NfjkYafd9mQrdBfv8Ruj2rJLGsTAYiVgE9hBl0W3vr/9WSJ6T 3v09VIxWlAuob6J1qTXm0lFYfDtYV9WfP3u1Y6GZIL3X1SMb0foMpSiKCU/vSxfS mcsIDaMH8tBeBx4tf6IpJzwazcfMs7rfVmgLJeHTaEI2l89fL0VeTXlNKHYCk7A= =/YSv -----END PGP SIGNATURE----- --=-Oq6r2pVxhtvyqeDjhZ0x--