* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 19:03 [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races Rafael Aquini
@ 2013-12-17 19:31 ` Rik van Riel
2013-12-17 20:41 ` Greg Thelen
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Rik van Riel @ 2013-12-17 19:31 UTC (permalink / raw)
To: Rafael Aquini; +Cc: linux-kernel, Andrew Morton, Davidlohr Bueso, Greg Thelen
On 12/17/2013 02:03 PM, Rafael Aquini wrote:
> After the locking semantics for the SysV IPC API got improved, a couple of
> IPC_RMID race windows were opened because we ended up dropping the
> 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> The spotted races got sorted out by re-introducing the old test within
> the racy critical sections.
>
> This patch introduces ipc_valid_object() to consolidate the way we cope with
> IPC_RMID races by using the same abstraction across the API implementation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Rik van Riel <riel@redhat.com>
--
All rights reversed
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 19:03 [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races Rafael Aquini
2013-12-17 19:31 ` Rik van Riel
@ 2013-12-17 20:41 ` Greg Thelen
2013-12-17 21:27 ` Davidlohr Bueso
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Greg Thelen @ 2013-12-17 20:41 UTC (permalink / raw)
To: Rafael Aquini; +Cc: linux-kernel, Andrew Morton, Davidlohr Bueso, Rik van Riel
On Tue, Dec 17 2013, Rafael Aquini wrote:
> After the locking semantics for the SysV IPC API got improved, a couple of
> IPC_RMID race windows were opened because we ended up dropping the
> 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> The spotted races got sorted out by re-introducing the old test within
> the racy critical sections.
>
> This patch introduces ipc_valid_object() to consolidate the way we cope with
> IPC_RMID races by using the same abstraction across the API implementation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Greg Thelen <gthelen@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 19:03 [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races Rafael Aquini
2013-12-17 19:31 ` Rik van Riel
2013-12-17 20:41 ` Greg Thelen
@ 2013-12-17 21:27 ` Davidlohr Bueso
2013-12-17 21:46 ` Rafael Aquini
2013-12-17 23:28 ` [PATCH v2] " Rafael Aquini
2013-12-18 20:33 ` [PATCH v3] " Rafael Aquini
4 siblings, 1 reply; 18+ messages in thread
From: Davidlohr Bueso @ 2013-12-17 21:27 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
Ccing Manfred.
On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> After the locking semantics for the SysV IPC API got improved, a couple of
> IPC_RMID race windows were opened because we ended up dropping the
> 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> The spotted races got sorted out by re-introducing the old test within
> the racy critical sections.
>
> This patch introduces ipc_valid_object() to consolidate the way we cope with
> IPC_RMID races by using the same abstraction across the API implementation.
This is certainly a good function to have. Some comments below.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> ipc/msg.c | 7 ++++---
> ipc/sem.c | 8 ++++----
> ipc/shm.c | 16 ++++++++--------
> ipc/util.h | 13 +++++++++++++
> 4 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 558aa91..8983ea5 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> goto out_unlock0;
>
> /* raced with RMID? */
> - if (msq->q_perm.deleted) {
> + if (!ipc_valid_object(&msq->q_perm)) {
> err = -EIDRM;
> goto out_unlock0;
> }
> @@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> ipc_lock_object(&msq->q_perm);
>
> ipc_rcu_putref(msq, ipc_rcu_free);
> - if (msq->q_perm.deleted) {
> + /* raced with RMID? */
> + if (!ipc_valid_object(&msq->q_perm)) {
> err = -EIDRM;
> goto out_unlock0;
> }
> @@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
> ipc_lock_object(&msq->q_perm);
>
> /* raced with RMID? */
> - if (msq->q_perm.deleted) {
> + if (!ipc_valid_object(&msq->q_perm)) {
> msg = ERR_PTR(-EIDRM);
> goto out_unlock0;
> }
> diff --git a/ipc/sem.c b/ipc/sem.c
> index db9d241..f4fad32 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
>
> sem_lock(sma, NULL, -1);
>
> - if (sma->sem_perm.deleted) {
> + if (!ipc_valid_object(&sma->sem_perm)) {
> sem_unlock(sma, -1);
> rcu_read_unlock();
> return -EIDRM;
> @@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> int i;
>
> sem_lock(sma, NULL, -1);
> - if (sma->sem_perm.deleted) {
> + if (!ipc_valid_object(&sma->sem_perm)) {
> err = -EIDRM;
> goto out_unlock;
> }
> @@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> goto out_rcu_wakeup;
>
> sem_lock(sma, NULL, -1);
> - if (sma->sem_perm.deleted) {
> + if (!ipc_valid_object(&sma->sem_perm)) {
> err = -EIDRM;
> goto out_unlock;
> }
> @@ -2068,7 +2068,7 @@ void exit_sem(struct task_struct *tsk)
>
> sem_lock(sma, NULL, -1);
> /* exit_sem raced with IPC_RMID, nothing to do */
> - if (sma->sem_perm.deleted) {
> + if (!ipc_valid_object(&sma->sem_perm)) {
> sem_unlock(sma, -1);
> rcu_read_unlock();
> continue;
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7a51443..1bc68f1 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> goto out_unlock1;
>
> ipc_lock_object(&shp->shm_perm);
> +
> + /* check if shm_destroy() is tearing down shp */
> + if (!ipc_valid_object(&shp->shm_perm)) {
> + err = -EIDRM;
> + goto out_unlock0;
> + }
> +
> if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
> kuid_t euid = current_euid();
> if (!uid_eq(euid, shp->shm_perm.uid) &&
> @@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> }
>
> shm_file = shp->shm_file;
> -
> - /* check if shm_destroy() is tearing down shp */
> - if (shm_file == NULL) {
> - err = -EIDRM;
> - goto out_unlock0;
> - }
Ok, this seems safe, we can always rely on .deleted for validity since
shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
change is really moving what we're checking against just a few
instructions later.
> -
> if (is_file_hugepages(shm_file))
> goto out_unlock0;
>
> @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> ipc_lock_object(&shp->shm_perm);
>
> /* check if shm_destroy() is tearing down shp */
> - if (shp->shm_file == NULL) {
> + if (!ipc_valid_object(&shp->shm_perm)) {
> ipc_unlock_object(&shp->shm_perm);
> err = -EIDRM;
> goto out_unlock;
> diff --git a/ipc/util.h b/ipc/util.h
> index 59d78aa..3a5f0d0 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -185,6 +185,19 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
> rcu_read_unlock();
> }
>
> +/*
> + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> + * where the respective ipc_ids.rwsem is not being held down.
> + * Checks whether the ipc object is still around or if it's gone already, as
> + * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> + * Needs to be called with kern_ipc_perm.lock held.
> + */
> +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> +{
> + assert_spin_locked(&perm->lock);
This is already guaranteed, we don't need it (not that it's much of an
overhead in any case). IMO the comment above explicitly saying to call
it with the ipc lock held is enough.
> + return perm->deleted == 0;
We should turn this into a bool (yes, that's another patch).
> +}
> +
> struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
> int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
> struct ipc_ops *ops, struct ipc_params *params);
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 21:27 ` Davidlohr Bueso
@ 2013-12-17 21:46 ` Rafael Aquini
2013-12-17 22:18 ` Davidlohr Bueso
0 siblings, 1 reply; 18+ messages in thread
From: Rafael Aquini @ 2013-12-17 21:46 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
On Tue, Dec 17, 2013 at 01:27:49PM -0800, Davidlohr Bueso wrote:
> Ccing Manfred.
>
> On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> > After the locking semantics for the SysV IPC API got improved, a couple of
> > IPC_RMID race windows were opened because we ended up dropping the
> > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > The spotted races got sorted out by re-introducing the old test within
> > the racy critical sections.
> >
> > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > IPC_RMID races by using the same abstraction across the API implementation.
>
> This is certainly a good function to have. Some comments below.
>
> >
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > ipc/msg.c | 7 ++++---
> > ipc/sem.c | 8 ++++----
> > ipc/shm.c | 16 ++++++++--------
> > ipc/util.h | 13 +++++++++++++
> > 4 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/ipc/msg.c b/ipc/msg.c
> > index 558aa91..8983ea5 100644
> > --- a/ipc/msg.c
> > +++ b/ipc/msg.c
> > @@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> > goto out_unlock0;
> >
> > /* raced with RMID? */
> > - if (msq->q_perm.deleted) {
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > err = -EIDRM;
> > goto out_unlock0;
> > }
> > @@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> > ipc_lock_object(&msq->q_perm);
> >
> > ipc_rcu_putref(msq, ipc_rcu_free);
> > - if (msq->q_perm.deleted) {
> > + /* raced with RMID? */
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > err = -EIDRM;
> > goto out_unlock0;
> > }
> > @@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
> > ipc_lock_object(&msq->q_perm);
> >
> > /* raced with RMID? */
> > - if (msq->q_perm.deleted) {
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > msg = ERR_PTR(-EIDRM);
> > goto out_unlock0;
> > }
> > diff --git a/ipc/sem.c b/ipc/sem.c
> > index db9d241..f4fad32 100644
> > --- a/ipc/sem.c
> > +++ b/ipc/sem.c
> > @@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
> >
> > sem_lock(sma, NULL, -1);
> >
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > sem_unlock(sma, -1);
> > rcu_read_unlock();
> > return -EIDRM;
> > @@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> > int i;
> >
> > sem_lock(sma, NULL, -1);
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > err = -EIDRM;
> > goto out_unlock;
> > }
> > @@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> > goto out_rcu_wakeup;
> >
> > sem_lock(sma, NULL, -1);
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > err = -EIDRM;
> > goto out_unlock;
> > }
> > @@ -2068,7 +2068,7 @@ void exit_sem(struct task_struct *tsk)
> >
> > sem_lock(sma, NULL, -1);
> > /* exit_sem raced with IPC_RMID, nothing to do */
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > sem_unlock(sma, -1);
> > rcu_read_unlock();
> > continue;
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 7a51443..1bc68f1 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> > goto out_unlock1;
> >
> > ipc_lock_object(&shp->shm_perm);
> > +
> > + /* check if shm_destroy() is tearing down shp */
> > + if (!ipc_valid_object(&shp->shm_perm)) {
> > + err = -EIDRM;
> > + goto out_unlock0;
> > + }
> > +
> > if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
> > kuid_t euid = current_euid();
> > if (!uid_eq(euid, shp->shm_perm.uid) &&
> > @@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> > }
> >
> > shm_file = shp->shm_file;
> > -
> > - /* check if shm_destroy() is tearing down shp */
> > - if (shm_file == NULL) {
> > - err = -EIDRM;
> > - goto out_unlock0;
> > - }
>
> Ok, this seems safe, we can always rely on .deleted for validity since
> shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
> change is really moving what we're checking against just a few
> instructions later.
>
Yep, I did change it cause it seems that there's no reason to delay the return
condition if we raced with shm_destroy(), anyways.
> > -
> > if (is_file_hugepages(shm_file))
> > goto out_unlock0;
> >
> > @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> > ipc_lock_object(&shp->shm_perm);
> >
> > /* check if shm_destroy() is tearing down shp */
> > - if (shp->shm_file == NULL) {
> > + if (!ipc_valid_object(&shp->shm_perm)) {
> > ipc_unlock_object(&shp->shm_perm);
> > err = -EIDRM;
> > goto out_unlock;
> > diff --git a/ipc/util.h b/ipc/util.h
> > index 59d78aa..3a5f0d0 100644
> > --- a/ipc/util.h
> > +++ b/ipc/util.h
> > @@ -185,6 +185,19 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
> > rcu_read_unlock();
> > }
> >
> > +/*
> > + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> > + * where the respective ipc_ids.rwsem is not being held down.
> > + * Checks whether the ipc object is still around or if it's gone already, as
> > + * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> > + * Needs to be called with kern_ipc_perm.lock held.
> > + */
> > +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> > +{
> > + assert_spin_locked(&perm->lock);
>
> This is already guaranteed, we don't need it (not that it's much of an
> overhead in any case). IMO the comment above explicitly saying to call
> it with the ipc lock held is enough.
>
Good thought. I'll drop the assert and cover one extra case in sem.c where the
check can be done lockless (just spotted it).
Will submit a V2 soon with the change above! Thanks for the review.
>
> > + return perm->deleted == 0;
>
> We should turn this into a bool (yes, that's another patch).
>
Yep, I thought doing the scalar-2-bool conversion here and submit another patch
later, for a total boolean convertion, when (if) this one gets accepted.
Thanks for your thoughts!
-- Rafael
> > +}
> > +
> > struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
> > int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
> > struct ipc_ops *ops, struct ipc_params *params);
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 21:46 ` Rafael Aquini
@ 2013-12-17 22:18 ` Davidlohr Bueso
2013-12-17 22:50 ` Rafael Aquini
0 siblings, 1 reply; 18+ messages in thread
From: Davidlohr Bueso @ 2013-12-17 22:18 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
On Tue, 2013-12-17 at 19:46 -0200, Rafael Aquini wrote:
> On Tue, Dec 17, 2013 at 01:27:49PM -0800, Davidlohr Bueso wrote:
> > Ccing Manfred.
> >
> > On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> > > After the locking semantics for the SysV IPC API got improved, a couple of
> > > IPC_RMID race windows were opened because we ended up dropping the
> > > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > > The spotted races got sorted out by re-introducing the old test within
> > > the racy critical sections.
> > >
> > > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > > IPC_RMID races by using the same abstraction across the API implementation.
> >
> > This is certainly a good function to have. Some comments below.
> >
[...]
> > >
> > > shm_file = shp->shm_file;
> > > -
> > > - /* check if shm_destroy() is tearing down shp */
> > > - if (shm_file == NULL) {
> > > - err = -EIDRM;
> > > - goto out_unlock0;
> > > - }
> >
> > Ok, this seems safe, we can always rely on .deleted for validity since
> > shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
> > change is really moving what we're checking against just a few
> > instructions later.
> >
>
> Yep, I did change it cause it seems that there's no reason to delay the return
> condition if we raced with shm_destroy(), anyways.
>
Right, but I was referring to moving what we consider as valid.
static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
{
struct file *shm_file;
shm_file = shp->shm_file;
shp->shm_file = NULL; <--- we currently use this.
ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT;
shm_rmid(ns, shp); <--- with your patch we now use this.
shm_unlock(shp);
...
}
... and it makes since since shm was the only one not using .deleted for
RMID racing checks.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 22:18 ` Davidlohr Bueso
@ 2013-12-17 22:50 ` Rafael Aquini
0 siblings, 0 replies; 18+ messages in thread
From: Rafael Aquini @ 2013-12-17 22:50 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
On Tue, Dec 17, 2013 at 02:18:02PM -0800, Davidlohr Bueso wrote:
> On Tue, 2013-12-17 at 19:46 -0200, Rafael Aquini wrote:
> > On Tue, Dec 17, 2013 at 01:27:49PM -0800, Davidlohr Bueso wrote:
> > > Ccing Manfred.
> > >
> > > On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> > > > After the locking semantics for the SysV IPC API got improved, a couple of
> > > > IPC_RMID race windows were opened because we ended up dropping the
> > > > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > > > The spotted races got sorted out by re-introducing the old test within
> > > > the racy critical sections.
> > > >
> > > > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > > > IPC_RMID races by using the same abstraction across the API implementation.
> > >
> > > This is certainly a good function to have. Some comments below.
> > >
> [...]
> > > >
> > > > shm_file = shp->shm_file;
> > > > -
> > > > - /* check if shm_destroy() is tearing down shp */
> > > > - if (shm_file == NULL) {
> > > > - err = -EIDRM;
> > > > - goto out_unlock0;
> > > > - }
> > >
> > > Ok, this seems safe, we can always rely on .deleted for validity since
> > > shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
> > > change is really moving what we're checking against just a few
> > > instructions later.
> > >
> >
> > Yep, I did change it cause it seems that there's no reason to delay the return
> > condition if we raced with shm_destroy(), anyways.
> >
>
> Right, but I was referring to moving what we consider as valid.
>
> static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
> {
> struct file *shm_file;
>
> shm_file = shp->shm_file;
> shp->shm_file = NULL; <--- we currently use this.
> ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT;
> shm_rmid(ns, shp); <--- with your patch we now use this.
> shm_unlock(shp);
> ...
> }
>
> ... and it makes since since shm was the only one not using .deleted for
> RMID racing checks.
>
Oh, I see. As a matter of fact, what made shm to start using a different check
than kern_ipc_perm.deleted, as you points out, was the follwoing commit:
---8<--
commit a399b29dfbaaaf91162b2dc5a5875dd51bbfa2a1
Author: Greg Thelen <gthelen@google.com>
Date: Thu Nov 21 14:32:00 2013 -0800
ipc,shm: fix shm_file deletion races
--->8---
Although it closes the spotted race properly, Greg's commit also implies that (by the
way it works the race around) the race has always been there, which is not true.
OTOH, I didn't propose reverting Greg's commit because I thought the changes
it introduced to shm_destroy() might come handy to help one spotting similar races,
if they eventually pop out in the future.
(a cleanup patch can be sent later, if that hunk is not regarded as useful anymore)
Thanks!
-- Rafael
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 19:03 [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races Rafael Aquini
` (2 preceding siblings ...)
2013-12-17 21:27 ` Davidlohr Bueso
@ 2013-12-17 23:28 ` Rafael Aquini
2013-12-18 12:11 ` Manfred Spraul
2013-12-18 20:33 ` [PATCH v3] " Rafael Aquini
4 siblings, 1 reply; 18+ messages in thread
From: Rafael Aquini @ 2013-12-17 23:28 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Davidlohr Bueso, Rik van Riel, Greg Thelen,
Manfred Spraul
After the locking semantics for the SysV IPC API got improved, a couple of
IPC_RMID race windows were opened because we ended up dropping the
'kern_ipc_perm.deleted' check performed way down in ipc_lock().
The spotted races got sorted out by re-introducing the old test within
the racy critical sections.
This patch introduces ipc_valid_object() to consolidate the way we cope with
IPC_RMID races by using the same abstraction across the API implementation.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Rik van Riel <riel@redhat.com>
Acked-by: Greg Thelen <gthelen@google.com>
---
Changelog:
* v2:
- drop assert_spin_locked() from ipc_valid_object() for less overhead
- extend ipc_valid_object() usage in sem.c (not spotted checkpoints)
- keep the received ACKs
ipc/msg.c | 7 ++++---
ipc/sem.c | 16 ++++++++--------
ipc/shm.c | 16 ++++++++--------
ipc/util.h | 12 ++++++++++++
4 files changed, 32 insertions(+), 19 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index 558aa91..8983ea5 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
goto out_unlock0;
/* raced with RMID? */
- if (msq->q_perm.deleted) {
+ if (!ipc_valid_object(&msq->q_perm)) {
err = -EIDRM;
goto out_unlock0;
}
@@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
ipc_lock_object(&msq->q_perm);
ipc_rcu_putref(msq, ipc_rcu_free);
- if (msq->q_perm.deleted) {
+ /* raced with RMID? */
+ if (!ipc_valid_object(&msq->q_perm)) {
err = -EIDRM;
goto out_unlock0;
}
@@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
ipc_lock_object(&msq->q_perm);
/* raced with RMID? */
- if (msq->q_perm.deleted) {
+ if (!ipc_valid_object(&msq->q_perm)) {
msg = ERR_PTR(-EIDRM);
goto out_unlock0;
}
diff --git a/ipc/sem.c b/ipc/sem.c
index db9d241..ed0057a 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
return -EIDRM;
@@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
int i;
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1361,7 +1361,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1409,7 +1409,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
}
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
goto out_rcu_wakeup;
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1699,7 +1699,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
/* step 3: Acquire the lock on semaphore array */
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
kfree(new);
@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
error = -EIDRM;
locknum = sem_lock(sma, sops, nsops);
- if (sma->sem_perm.deleted)
+ if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
/*
* semid identifiers are not unique - find_alloc_undo may have
@@ -2068,7 +2068,7 @@ void exit_sem(struct task_struct *tsk)
sem_lock(sma, NULL, -1);
/* exit_sem raced with IPC_RMID, nothing to do */
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
continue;
diff --git a/ipc/shm.c b/ipc/shm.c
index 7a51443..1bc68f1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
goto out_unlock1;
ipc_lock_object(&shp->shm_perm);
+
+ /* check if shm_destroy() is tearing down shp */
+ if (!ipc_valid_object(&shp->shm_perm)) {
+ err = -EIDRM;
+ goto out_unlock0;
+ }
+
if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
kuid_t euid = current_euid();
if (!uid_eq(euid, shp->shm_perm.uid) &&
@@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
}
shm_file = shp->shm_file;
-
- /* check if shm_destroy() is tearing down shp */
- if (shm_file == NULL) {
- err = -EIDRM;
- goto out_unlock0;
- }
-
if (is_file_hugepages(shm_file))
goto out_unlock0;
@@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
ipc_lock_object(&shp->shm_perm);
/* check if shm_destroy() is tearing down shp */
- if (shp->shm_file == NULL) {
+ if (!ipc_valid_object(&shp->shm_perm)) {
ipc_unlock_object(&shp->shm_perm);
err = -EIDRM;
goto out_unlock;
diff --git a/ipc/util.h b/ipc/util.h
index 59d78aa..071ed58 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -185,6 +185,18 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
rcu_read_unlock();
}
+/*
+ * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
+ * where the respective ipc_ids.rwsem is not being held down.
+ * Checks whether the ipc object is still around or if it's gone already, as
+ * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
+ * Needs to be called with kern_ipc_perm.lock held.
+ */
+static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
+{
+ return perm->deleted == 0;
+}
+
struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
struct ipc_ops *ops, struct ipc_params *params);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 23:28 ` [PATCH v2] " Rafael Aquini
@ 2013-12-18 12:11 ` Manfred Spraul
2013-12-18 12:51 ` Rafael Aquini
0 siblings, 1 reply; 18+ messages in thread
From: Manfred Spraul @ 2013-12-18 12:11 UTC (permalink / raw)
To: Rafael Aquini, linux-kernel
Cc: Andrew Morton, Davidlohr Bueso, Rik van Riel, Greg Thelen
On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> After the locking semantics for the SysV IPC API got improved, a couple of
> IPC_RMID race windows were opened because we ended up dropping the
> 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> The spotted races got sorted out by re-introducing the old test within
> the racy critical sections.
>
> This patch introduces ipc_valid_object() to consolidate the way we cope with
> IPC_RMID races by using the same abstraction across the API implementation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> Acked-by: Greg Thelen <gthelen@google.com>
> ---
> Changelog:
> * v2:
> - drop assert_spin_locked() from ipc_valid_object() for less overhead
a) sysv ipc is lockless whereever possible, without writing to any
shared cachelines.
Therefore my first reaction was: No, please leave the assert in. It will
help us to catch bugs.
b) then I noticed: the assert would be a bug, the comment in front of
ipc_valid_object() that the caller must hold _perm.lock is wrong:
> @@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
>
> error = -EIDRM;
> locknum = sem_lock(sma, sops, nsops);
> - if (sma->sem_perm.deleted)
> + if (!ipc_valid_object(&sma->sem_perm))
> goto out_unlock_free;
simple semtimedop() operation do not acquire sem_perm.lock, they only
acquire the per-semaphore lock and check that sem_perm.lock is not held.
This is sufficient to prevent races with RMID.
Could you update the comment?
[...]
> @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> ipc_lock_object(&shp->shm_perm);
>
> /* check if shm_destroy() is tearing down shp */
> - if (shp->shm_file == NULL) {
> + if (!ipc_valid_object(&shp->shm_perm)) {
> ipc_unlock_object(&shp->shm_perm);
> err = -EIDRM;
> goto out_unlock;
Please mention the change from "shm_file == NULL" to perm.deleted in the
changelog.
With regards to the impact of this change: No idea, I've never worked on
the shm code.
--
Manfred
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 12:11 ` Manfred Spraul
@ 2013-12-18 12:51 ` Rafael Aquini
2013-12-18 13:12 ` Rafael Aquini
2013-12-18 15:46 ` Davidlohr Bueso
0 siblings, 2 replies; 18+ messages in thread
From: Rafael Aquini @ 2013-12-18 12:51 UTC (permalink / raw)
To: Manfred Spraul
Cc: linux-kernel, Andrew Morton, Davidlohr Bueso, Rik van Riel,
Greg Thelen
On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> >After the locking semantics for the SysV IPC API got improved, a couple of
> >IPC_RMID race windows were opened because we ended up dropping the
> >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> >The spotted races got sorted out by re-introducing the old test within
> >the racy critical sections.
> >
> >This patch introduces ipc_valid_object() to consolidate the way we cope with
> >IPC_RMID races by using the same abstraction across the API implementation.
> >
> >Signed-off-by: Rafael Aquini <aquini@redhat.com>
> >Acked-by: Rik van Riel <riel@redhat.com>
> >Acked-by: Greg Thelen <gthelen@google.com>
> >---
> >Changelog:
> >* v2:
> > - drop assert_spin_locked() from ipc_valid_object() for less overhead
> a) sysv ipc is lockless whereever possible, without writing to any
> shared cachelines.
> Therefore my first reaction was: No, please leave the assert in. It
> will help us to catch bugs.
>
> b) then I noticed: the assert would be a bug, the comment in front
> of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > error = -EIDRM;
> > locknum = sem_lock(sma, sops, nsops);
> >- if (sma->sem_perm.deleted)
> >+ if (!ipc_valid_object(&sma->sem_perm))
> > goto out_unlock_free;
> simple semtimedop() operation do not acquire sem_perm.lock, they
> only acquire the per-semaphore lock and check that sem_perm.lock is
> not held. This is sufficient to prevent races with RMID.
>
> Could you update the comment?
The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock
is clearly necessary for all cases except for this one you pointed above.
When I dropped the assert as Davilohr suggested, I then could have this one exception
case (where the check can, eventually, be done lockless) converted too, but I did not include
an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
perhaps the best thing is to just let all that as is sits right now.
> [...]
> >@@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> > ipc_lock_object(&shp->shm_perm);
> > /* check if shm_destroy() is tearing down shp */
> >- if (shp->shm_file == NULL) {
> >+ if (!ipc_valid_object(&shp->shm_perm)) {
> > ipc_unlock_object(&shp->shm_perm);
> > err = -EIDRM;
> > goto out_unlock;
> Please mention the change from "shm_file == NULL" to perm.deleted in
> the changelog.
> With regards to the impact of this change: No idea, I've never
> worked on the shm code.
This change is, essentially, the proper way to cope with such races. Please
refer to the following reply on this same trhead, for further info:
https://lkml.org/lkml/2013/12/17/704
Thanks!
-- Rafael
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 12:51 ` Rafael Aquini
@ 2013-12-18 13:12 ` Rafael Aquini
2013-12-18 15:46 ` Davidlohr Bueso
1 sibling, 0 replies; 18+ messages in thread
From: Rafael Aquini @ 2013-12-18 13:12 UTC (permalink / raw)
To: Manfred Spraul
Cc: linux-kernel, Andrew Morton, Davidlohr Bueso, Rik van Riel,
Greg Thelen
On Wed, Dec 18, 2013 at 10:50:59AM -0200, Rafael Aquini wrote:
> On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> > On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> > >After the locking semantics for the SysV IPC API got improved, a couple of
> > >IPC_RMID race windows were opened because we ended up dropping the
> > >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > >The spotted races got sorted out by re-introducing the old test within
> > >the racy critical sections.
> > >
> > >This patch introduces ipc_valid_object() to consolidate the way we cope with
> > >IPC_RMID races by using the same abstraction across the API implementation.
> > >
> > >Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > >Acked-by: Rik van Riel <riel@redhat.com>
> > >Acked-by: Greg Thelen <gthelen@google.com>
> > >---
> > >Changelog:
> > >* v2:
> > > - drop assert_spin_locked() from ipc_valid_object() for less overhead
> > a) sysv ipc is lockless whereever possible, without writing to any
> > shared cachelines.
> > Therefore my first reaction was: No, please leave the assert in. It
> > will help us to catch bugs.
> >
> > b) then I noticed: the assert would be a bug, the comment in front
> > of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > > error = -EIDRM;
> > > locknum = sem_lock(sma, sops, nsops);
> > >- if (sma->sem_perm.deleted)
> > >+ if (!ipc_valid_object(&sma->sem_perm))
> > > goto out_unlock_free;
> > simple semtimedop() operation do not acquire sem_perm.lock, they
> > only acquire the per-semaphore lock and check that sem_perm.lock is
> > not held. This is sufficient to prevent races with RMID.
> >
> > Could you update the comment?
>
> The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock
> is clearly necessary for all cases except for this one you pointed above.
> When I dropped the assert as Davilohr suggested, I then could have this one exception
> case (where the check can, eventually, be done lockless) converted too, but I did not include
> an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
> perhaps the best thing is to just let all that as is sits right now.
>
Or, as a second thought, we could perhaps re-instate the assert in
ipc_valid_object(), and change only this exception checkpoint back to a
if (sma->sem_perm.deleted) case, adding a comment there on why it's different
from the others.
Looking up to hear your thoughts here!
Thanks!
-- Rafael
>
> > [...]
> > >@@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> > > ipc_lock_object(&shp->shm_perm);
> > > /* check if shm_destroy() is tearing down shp */
> > >- if (shp->shm_file == NULL) {
> > >+ if (!ipc_valid_object(&shp->shm_perm)) {
> > > ipc_unlock_object(&shp->shm_perm);
> > > err = -EIDRM;
> > > goto out_unlock;
> > Please mention the change from "shm_file == NULL" to perm.deleted in
> > the changelog.
> > With regards to the impact of this change: No idea, I've never
> > worked on the shm code.
>
> This change is, essentially, the proper way to cope with such races. Please
> refer to the following reply on this same trhead, for further info:
> https://lkml.org/lkml/2013/12/17/704
>
> Thanks!
> -- Rafael
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 12:51 ` Rafael Aquini
2013-12-18 13:12 ` Rafael Aquini
@ 2013-12-18 15:46 ` Davidlohr Bueso
2013-12-18 15:53 ` Rafael Aquini
2013-12-18 17:34 ` Rafael Aquini
1 sibling, 2 replies; 18+ messages in thread
From: Davidlohr Bueso @ 2013-12-18 15:46 UTC (permalink / raw)
To: Rafael Aquini
Cc: Manfred Spraul, linux-kernel, Andrew Morton, Rik van Riel,
Greg Thelen
On Wed, 2013-12-18 at 10:51 -0200, Rafael Aquini wrote:
> On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> > On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> > >After the locking semantics for the SysV IPC API got improved, a couple of
> > >IPC_RMID race windows were opened because we ended up dropping the
> > >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > >The spotted races got sorted out by re-introducing the old test within
> > >the racy critical sections.
> > >
> > >This patch introduces ipc_valid_object() to consolidate the way we cope with
> > >IPC_RMID races by using the same abstraction across the API implementation.
> > >
> > >Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > >Acked-by: Rik van Riel <riel@redhat.com>
> > >Acked-by: Greg Thelen <gthelen@google.com>
> > >---
> > >Changelog:
> > >* v2:
> > > - drop assert_spin_locked() from ipc_valid_object() for less overhead
> > a) sysv ipc is lockless whereever possible, without writing to any
> > shared cachelines.
> > Therefore my first reaction was: No, please leave the assert in. It
> > will help us to catch bugs.
> >
> > b) then I noticed: the assert would be a bug, the comment in front
> > of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > > error = -EIDRM;
> > > locknum = sem_lock(sma, sops, nsops);
> > >- if (sma->sem_perm.deleted)
> > >+ if (!ipc_valid_object(&sma->sem_perm))
> > > goto out_unlock_free;
> > simple semtimedop() operation do not acquire sem_perm.lock, they
> > only acquire the per-semaphore lock and check that sem_perm.lock is
> > not held. This is sufficient to prevent races with RMID.
> >
> > Could you update the comment?
>
> The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock
> is clearly necessary for all cases except for this one you pointed above.
> When I dropped the assert as Davilohr suggested, I then could have this one exception
> case (where the check can, eventually, be done lockless) converted too, but I did not include
> an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
> perhaps the best thing is to just let all that as is sits right now.
Yeah, Manfred is entirely correct - I didn't mention that sem_lock()
tries to be fine grained about its locking, so semaphores can in fact
not take the larger ipc lock (kern perm), but just the sem->lock
instead. This means that ipc_valid_object() must be called either way
with some lock held, but that assertion is indeed incorrect, not just
redundant like I suggested before. So, I think that if you update the
comment mentioning this corner case, then it should be ok.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 15:46 ` Davidlohr Bueso
@ 2013-12-18 15:53 ` Rafael Aquini
2013-12-18 17:34 ` Rafael Aquini
1 sibling, 0 replies; 18+ messages in thread
From: Rafael Aquini @ 2013-12-18 15:53 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Manfred Spraul, linux-kernel, Andrew Morton, Rik van Riel,
Greg Thelen
On Wed, Dec 18, 2013 at 07:46:27AM -0800, Davidlohr Bueso wrote:
> On Wed, 2013-12-18 at 10:51 -0200, Rafael Aquini wrote:
> > On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> > > On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> > > >After the locking semantics for the SysV IPC API got improved, a couple of
> > > >IPC_RMID race windows were opened because we ended up dropping the
> > > >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > > >The spotted races got sorted out by re-introducing the old test within
> > > >the racy critical sections.
> > > >
> > > >This patch introduces ipc_valid_object() to consolidate the way we cope with
> > > >IPC_RMID races by using the same abstraction across the API implementation.
> > > >
> > > >Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > > >Acked-by: Rik van Riel <riel@redhat.com>
> > > >Acked-by: Greg Thelen <gthelen@google.com>
> > > >---
> > > >Changelog:
> > > >* v2:
> > > > - drop assert_spin_locked() from ipc_valid_object() for less overhead
> > > a) sysv ipc is lockless whereever possible, without writing to any
> > > shared cachelines.
> > > Therefore my first reaction was: No, please leave the assert in. It
> > > will help us to catch bugs.
> > >
> > > b) then I noticed: the assert would be a bug, the comment in front
> > > of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> > > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > > > error = -EIDRM;
> > > > locknum = sem_lock(sma, sops, nsops);
> > > >- if (sma->sem_perm.deleted)
> > > >+ if (!ipc_valid_object(&sma->sem_perm))
> > > > goto out_unlock_free;
> > > simple semtimedop() operation do not acquire sem_perm.lock, they
> > > only acquire the per-semaphore lock and check that sem_perm.lock is
> > > not held. This is sufficient to prevent races with RMID.
> > >
> > > Could you update the comment?
> >
> > The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock
> > is clearly necessary for all cases except for this one you pointed above.
> > When I dropped the assert as Davilohr suggested, I then could have this one exception
> > case (where the check can, eventually, be done lockless) converted too, but I did not include
> > an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
> > perhaps the best thing is to just let all that as is sits right now.
>
> Yeah, Manfred is entirely correct - I didn't mention that sem_lock()
> tries to be fine grained about its locking, so semaphores can in fact
> not take the larger ipc lock (kern perm), but just the sem->lock
> instead. This means that ipc_valid_object() must be called either way
> with some lock held, but that assertion is indeed incorrect, not just
> redundant like I suggested before. So, I think that if you update the
> comment mentioning this corner case, then it should be ok.
>
Cool, will do it, then. But I'll do it just above the exception case, in sem.c
to not cause more confusion. Does it sounds good to all?
Thanks, folks!
-- Rafael
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 15:46 ` Davidlohr Bueso
2013-12-18 15:53 ` Rafael Aquini
@ 2013-12-18 17:34 ` Rafael Aquini
2013-12-18 19:00 ` Manfred Spraul
1 sibling, 1 reply; 18+ messages in thread
From: Rafael Aquini @ 2013-12-18 17:34 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Manfred Spraul, linux-kernel, Andrew Morton, Rik van Riel,
Greg Thelen
On Wed, Dec 18, 2013 at 07:46:27AM -0800, Davidlohr Bueso wrote:
> On Wed, 2013-12-18 at 10:51 -0200, Rafael Aquini wrote:
> > On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> > > On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> > > >After the locking semantics for the SysV IPC API got improved, a couple of
> > > >IPC_RMID race windows were opened because we ended up dropping the
> > > >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > > >The spotted races got sorted out by re-introducing the old test within
> > > >the racy critical sections.
> > > >
> > > >This patch introduces ipc_valid_object() to consolidate the way we cope with
> > > >IPC_RMID races by using the same abstraction across the API implementation.
> > > >
> > > >Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > > >Acked-by: Rik van Riel <riel@redhat.com>
> > > >Acked-by: Greg Thelen <gthelen@google.com>
> > > >---
> > > >Changelog:
> > > >* v2:
> > > > - drop assert_spin_locked() from ipc_valid_object() for less overhead
> > > a) sysv ipc is lockless whereever possible, without writing to any
> > > shared cachelines.
> > > Therefore my first reaction was: No, please leave the assert in. It
> > > will help us to catch bugs.
> > >
> > > b) then I noticed: the assert would be a bug, the comment in front
> > > of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> > > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > > > error = -EIDRM;
> > > > locknum = sem_lock(sma, sops, nsops);
> > > >- if (sma->sem_perm.deleted)
> > > >+ if (!ipc_valid_object(&sma->sem_perm))
> > > > goto out_unlock_free;
> > > simple semtimedop() operation do not acquire sem_perm.lock, they
> > > only acquire the per-semaphore lock and check that sem_perm.lock is
> > > not held. This is sufficient to prevent races with RMID.
> > >
> > > Could you update the comment?
> >
> > The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock
> > is clearly necessary for all cases except for this one you pointed above.
> > When I dropped the assert as Davilohr suggested, I then could have this one exception
> > case (where the check can, eventually, be done lockless) converted too, but I did not include
> > an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
> > perhaps the best thing is to just let all that as is sits right now.
>
> Yeah, Manfred is entirely correct - I didn't mention that sem_lock()
> tries to be fine grained about its locking, so semaphores can in fact
> not take the larger ipc lock (kern perm), but just the sem->lock
> instead. This means that ipc_valid_object() must be called either way
> with some lock held, but that assertion is indeed incorrect, not just
> redundant like I suggested before. So, I think that if you update the
> comment mentioning this corner case, then it should be ok.
>
Folks,
Before I re-submit the v3 with the commentary changes requested, I'm pasting
here what I'm planning to amend to v2 patch:
---
diff --git a/ipc/sem.c b/ipc/sem.c
index ed0057a..23379b6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1846,6 +1846,14 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __u
error = -EIDRM;
locknum = sem_lock(sma, sops, nsops);
+ /*
+ * We eventually might perform the following check in a lockless
+ * fashion here, considering ipc_valid_object() locking constraints.
+ * If nsops == 1 and there's no contention for sem_perm.lock, then
+ * only a per-semaphore lock is held and it's OK to go on the check
+ * below. More details on the fine grained locking scheme entangled
+ * here, and why it's RMID race safe on comments at sem_lock()
+ */
if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
/*
diff --git a/ipc/util.h b/ipc/util.h
index 071ed58..d05b708 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -190,7 +190,8 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
* where the respective ipc_ids.rwsem is not being held down.
* Checks whether the ipc object is still around or if it's gone already, as
* ipc_rmid() may have already freed the ID while the ipc lock was spinning.
- * Needs to be called with kern_ipc_perm.lock held.
+ * Needs to be called with kern_ipc_perm.lock held -- exception made for one
+ * checkpoint case at sys_semtimedop() as noted in code commentary.
*/
static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
{
---
Do we need to change somthing else?
Looking forward your thoughts!
-- Rafael
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 17:34 ` Rafael Aquini
@ 2013-12-18 19:00 ` Manfred Spraul
0 siblings, 0 replies; 18+ messages in thread
From: Manfred Spraul @ 2013-12-18 19:00 UTC (permalink / raw)
To: Rafael Aquini, Davidlohr Bueso
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen
Hi Rafael,
On 12/18/2013 06:34 PM, Rafael Aquini wrote:
> Folks,
>
> Before I re-submit the v3 with the commentary changes requested, I'm pasting
> here what I'm planning to amend to v2 patch:
> ---
> diff --git a/ipc/sem.c b/ipc/sem.c
> index ed0057a..23379b6 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1846,6 +1846,14 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __u
>
> error = -EIDRM;
> locknum = sem_lock(sma, sops, nsops);
> + /*
> + * We eventually might perform the following check in a lockless
> + * fashion here, considering ipc_valid_object() locking constraints.
> + * If nsops == 1 and there's no contention for sem_perm.lock, then
> + * only a per-semaphore lock is held and it's OK to go on the check
> + * below. More details on the fine grained locking scheme entangled
> + * here, and why it's RMID race safe on comments at sem_lock()
> + */
> if (!ipc_valid_object(&sma->sem_perm))
> goto out_unlock_free;
> /*
> diff --git a/ipc/util.h b/ipc/util.h
> index 071ed58..d05b708 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -190,7 +190,8 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
> * where the respective ipc_ids.rwsem is not being held down.
> * Checks whether the ipc object is still around or if it's gone already, as
> * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> - * Needs to be called with kern_ipc_perm.lock held.
> + * Needs to be called with kern_ipc_perm.lock held -- exception made for one
> + * checkpoint case at sys_semtimedop() as noted in code commentary.
> */
> static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> {
> ---
>
> Do we need to change somthing else?
> Looking forward your thoughts!
Acked-by: Manfred Spraul <manfred@colorfullife.com>
--
Manfred
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-17 19:03 [PATCH] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races Rafael Aquini
` (3 preceding siblings ...)
2013-12-17 23:28 ` [PATCH v2] " Rafael Aquini
@ 2013-12-18 20:33 ` Rafael Aquini
2013-12-19 0:38 ` Davidlohr Bueso
4 siblings, 1 reply; 18+ messages in thread
From: Rafael Aquini @ 2013-12-18 20:33 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Davidlohr Bueso, Rik van Riel, Greg Thelen,
Manfred Spraul
After the locking semantics for the SysV IPC API got improved, a couple of
IPC_RMID race windows were opened because we ended up dropping the
'kern_ipc_perm.deleted' check performed way down in ipc_lock().
The spotted races got sorted out by re-introducing the old test within
the racy critical sections.
This patch introduces ipc_valid_object() to consolidate the way we cope with
IPC_RMID races by using the same abstraction across the API implementation.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Rik van Riel <riel@redhat.com>
Acked-by: Greg Thelen <gthelen@google.com>
---
Changelog:
* v3:
- code commentary changes as requested by reviewers
* v2:
- drop assert_spin_locked() from ipc_valid_object() for less overhead
- extend ipc_valid_object() usage in sem.c (not spotted checkpoints)
- keep the received ACKs
ipc/msg.c | 7 ++++---
ipc/sem.c | 24 ++++++++++++++++--------
ipc/shm.c | 16 ++++++++--------
ipc/util.h | 13 +++++++++++++
4 files changed, 41 insertions(+), 19 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index 558aa91..8983ea5 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
goto out_unlock0;
/* raced with RMID? */
- if (msq->q_perm.deleted) {
+ if (!ipc_valid_object(&msq->q_perm)) {
err = -EIDRM;
goto out_unlock0;
}
@@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
ipc_lock_object(&msq->q_perm);
ipc_rcu_putref(msq, ipc_rcu_free);
- if (msq->q_perm.deleted) {
+ /* raced with RMID? */
+ if (!ipc_valid_object(&msq->q_perm)) {
err = -EIDRM;
goto out_unlock0;
}
@@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
ipc_lock_object(&msq->q_perm);
/* raced with RMID? */
- if (msq->q_perm.deleted) {
+ if (!ipc_valid_object(&msq->q_perm)) {
msg = ERR_PTR(-EIDRM);
goto out_unlock0;
}
diff --git a/ipc/sem.c b/ipc/sem.c
index db9d241..5972e60 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
return -EIDRM;
@@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
int i;
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1361,7 +1361,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1409,7 +1409,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
}
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
goto out_rcu_wakeup;
sem_lock(sma, NULL, -1);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
err = -EIDRM;
goto out_unlock;
}
@@ -1699,7 +1699,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
/* step 3: Acquire the lock on semaphore array */
rcu_read_lock();
sem_lock_and_putref(sma);
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
kfree(new);
@@ -1846,7 +1846,15 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
error = -EIDRM;
locknum = sem_lock(sma, sops, nsops);
- if (sma->sem_perm.deleted)
+ /*
+ * We eventually might perform the following check in a lockless
+ * fashion, considering ipc_valid_object() locking constraints.
+ * If nsops == 1 and there is no contention for sem_perm.lock, then
+ * only a per-semaphore lock is held and it's OK to proceed with the
+ * check below. More details on the fine grained locking scheme
+ * entangled here and why it's RMID race safe on comments at sem_lock()
+ */
+ if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
/*
* semid identifiers are not unique - find_alloc_undo may have
@@ -2068,7 +2076,7 @@ void exit_sem(struct task_struct *tsk)
sem_lock(sma, NULL, -1);
/* exit_sem raced with IPC_RMID, nothing to do */
- if (sma->sem_perm.deleted) {
+ if (!ipc_valid_object(&sma->sem_perm)) {
sem_unlock(sma, -1);
rcu_read_unlock();
continue;
diff --git a/ipc/shm.c b/ipc/shm.c
index 7a51443..1bc68f1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
goto out_unlock1;
ipc_lock_object(&shp->shm_perm);
+
+ /* check if shm_destroy() is tearing down shp */
+ if (!ipc_valid_object(&shp->shm_perm)) {
+ err = -EIDRM;
+ goto out_unlock0;
+ }
+
if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
kuid_t euid = current_euid();
if (!uid_eq(euid, shp->shm_perm.uid) &&
@@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
}
shm_file = shp->shm_file;
-
- /* check if shm_destroy() is tearing down shp */
- if (shm_file == NULL) {
- err = -EIDRM;
- goto out_unlock0;
- }
-
if (is_file_hugepages(shm_file))
goto out_unlock0;
@@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
ipc_lock_object(&shp->shm_perm);
/* check if shm_destroy() is tearing down shp */
- if (shp->shm_file == NULL) {
+ if (!ipc_valid_object(&shp->shm_perm)) {
ipc_unlock_object(&shp->shm_perm);
err = -EIDRM;
goto out_unlock;
diff --git a/ipc/util.h b/ipc/util.h
index 59d78aa..d05b708 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -185,6 +185,19 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
rcu_read_unlock();
}
+/*
+ * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
+ * where the respective ipc_ids.rwsem is not being held down.
+ * Checks whether the ipc object is still around or if it's gone already, as
+ * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
+ * Needs to be called with kern_ipc_perm.lock held -- exception made for one
+ * checkpoint case at sys_semtimedop() as noted in code commentary.
+ */
+static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
+{
+ return perm->deleted == 0;
+}
+
struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
struct ipc_ops *ops, struct ipc_params *params);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-18 20:33 ` [PATCH v3] " Rafael Aquini
@ 2013-12-19 0:38 ` Davidlohr Bueso
2013-12-19 0:42 ` Rafael Aquini
0 siblings, 1 reply; 18+ messages in thread
From: Davidlohr Bueso @ 2013-12-19 0:38 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
On Wed, 2013-12-18 at 18:33 -0200, Rafael Aquini wrote:
> After the locking semantics for the SysV IPC API got improved, a couple of
> IPC_RMID race windows were opened because we ended up dropping the
> 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> The spotted races got sorted out by re-introducing the old test within
> the racy critical sections.
>
> This patch introduces ipc_valid_object() to consolidate the way we cope with
> IPC_RMID races by using the same abstraction across the API implementation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> Acked-by: Greg Thelen <gthelen@google.com>
Reviewed-by: Davidlohr Bueso <davidlohr@hp.com>
[...]
> +/*
> + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> + * where the respective ipc_ids.rwsem is not being held down.
> + * Checks whether the ipc object is still around or if it's gone already, as
> + * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> + * Needs to be called with kern_ipc_perm.lock held -- exception made for one
> + * checkpoint case at sys_semtimedop() as noted in code commentary.
> + */
> +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> +{
> + return perm->deleted == 0;
> +}
I would like to see .deleted being converted to bool while we're at it
though, that return statement just bugs the hell out of me. Could you
send a patch for that as well?
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races
2013-12-19 0:38 ` Davidlohr Bueso
@ 2013-12-19 0:42 ` Rafael Aquini
0 siblings, 0 replies; 18+ messages in thread
From: Rafael Aquini @ 2013-12-19 0:42 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-kernel, Andrew Morton, Rik van Riel, Greg Thelen,
Manfred Spraul
On Wed, Dec 18, 2013 at 04:38:24PM -0800, Davidlohr Bueso wrote:
> On Wed, 2013-12-18 at 18:33 -0200, Rafael Aquini wrote:
> > After the locking semantics for the SysV IPC API got improved, a couple of
> > IPC_RMID race windows were opened because we ended up dropping the
> > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > The spotted races got sorted out by re-introducing the old test within
> > the racy critical sections.
> >
> > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > IPC_RMID races by using the same abstraction across the API implementation.
> >
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > Acked-by: Rik van Riel <riel@redhat.com>
> > Acked-by: Greg Thelen <gthelen@google.com>
>
> Reviewed-by: Davidlohr Bueso <davidlohr@hp.com>
>
> [...]
>
> > +/*
> > + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> > + * where the respective ipc_ids.rwsem is not being held down.
> > + * Checks whether the ipc object is still around or if it's gone already, as
> > + * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> > + * Needs to be called with kern_ipc_perm.lock held -- exception made for one
> > + * checkpoint case at sys_semtimedop() as noted in code commentary.
> > + */
> > +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> > +{
> > + return perm->deleted == 0;
> > +}
>
> I would like to see .deleted being converted to bool while we're at it
> though, that return statement just bugs the hell out of me. Could you
> send a patch for that as well?
>
Sure, as I mentioned earlier the full .deleted conversion from int to bool
it's on my todo list already for a follow-up patch.
Thanks!
-- Rafael
^ permalink raw reply [flat|nested] 18+ messages in thread