[lttng-dev] [PATCH lttng-ust 2/2] Use ust_lock on fd tracker add operation for atomicity against fork

[Jonathan Rajotte-Julien]

Hi,

Regarding double lock, ust_lock is nestable.

Still, you are quite right regarding the fact that this patch is useless. Not sure
what I was thinking at that time.

Cheers

On Thu, Feb 01, 2018 at 10:52:43PM +0000, Mathieu Desnoyers wrote:
> 
> ----- On Feb 1, 2018, at 5:28 PM, Jonathan Rajotte jonathan.rajotte-julien at efficios.com wrote:
> 
> > Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> > ---
> > liblttng-ust/lttng-ust-comm.c | 8 ++++++++
> > liblttng-ust/lttng-ust-elf.c  | 8 ++++++++
> > 2 files changed, 16 insertions(+)
> > 
> > diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
> > index 32ba3c1..f4b7b53 100644
> > --- a/liblttng-ust/lttng-ust-comm.c
> > +++ b/liblttng-ust/lttng-ust-comm.c
> > @@ -1255,10 +1255,16 @@ char *get_map_shm(struct sock_info *sock_info)
> > 		goto error;
> > 	}
> > 
> > +	if (ust_lock()) {
> > +		ust_unlock();
> > +		goto error;
> > +	}
> 
> get_map_shm() is called from a context that already holds the ust_lock().
> 
> This is a double-lock...
> 
> 
> > +
> > 	lttng_ust_lock_fd_tracker();
> > 	wait_shm_fd = get_wait_shm(sock_info, page_size);
> > 	if (wait_shm_fd < 0) {
> > 		lttng_ust_unlock_fd_tracker();
> > +		ust_unlock();
> > 		goto error;
> > 	}
> > 
> > @@ -1269,11 +1275,13 @@ char *get_map_shm(struct sock_info *sock_info)
> > 			PERROR("Error closing fd");
> > 		}
> > 		lttng_ust_unlock_fd_tracker();
> > +		ust_unlock();
> > 		goto error;
> > 	}
> > 
> > 	wait_shm_fd = ret;
> > 	lttng_ust_unlock_fd_tracker();
> > +	ust_unlock();
> > 
> > 	wait_shm_mmap = mmap(NULL, page_size, PROT_READ,
> > 		  MAP_SHARED, wait_shm_fd, 0);
> > diff --git a/liblttng-ust/lttng-ust-elf.c b/liblttng-ust/lttng-ust-elf.c
> > index c073e7a..03c715a 100644
> > --- a/liblttng-ust/lttng-ust-elf.c
> > +++ b/liblttng-ust/lttng-ust-elf.c
> > @@ -256,10 +256,16 @@ struct lttng_ust_elf *lttng_ust_elf_create(const char
> > *path)
> > 		goto error;
> > 	}
> > 
> > +	if (ust_lock()) {
> > +		ust_unlock();
> > +		goto error;
> > +	}
> 
> same here. iter_begin in liblttng-ust/lttng-ust-statedump.c already grabs the
> ust lock. This won't work.
> 
> I wonder if any part of this patch is needed ?
> 
> Thanks,
> 
> Mathieu
> 
> > +
> > 	lttng_ust_lock_fd_tracker();
> > 	fd = open(elf->path, O_RDONLY | O_CLOEXEC);
> > 	if (fd < 0) {
> > 		lttng_ust_unlock_fd_tracker();
> > +		ust_unlock();
> > 		goto error;
> > 	}
> > 
> > @@ -271,10 +277,12 @@ struct lttng_ust_elf *lttng_ust_elf_create(const char
> > *path)
> > 		}
> > 		ret = -1;
> > 		lttng_ust_unlock_fd_tracker();
> > +		ust_unlock();
> > 		goto error;
> > 	}
> > 	elf->fd = ret;
> > 	lttng_ust_unlock_fd_tracker();
> > +	ust_unlock();
> > 
> > 	if (lttng_ust_read(elf->fd, e_ident, EI_NIDENT) < EI_NIDENT) {
> > 		goto error;
> > --
> > 2.7.4
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

-- 
Jonathan Rajotte-Julien
EfficiOS