Hi Gilles,
while rebasing my half-done ftrace instrumentation over next, I got
stuck reading nsem_open. Find my questions inline:
> static struct __shadow_sem __user *
> nsem_open(struct __shadow_sem __user *ushadow, const char *name,
> int oflags, mode_t mode, unsigned value)
> {
> struct __shadow_sem shadow;
> struct cobalt_sem *sem;
> struct nsem *u, *v;
> xnhandle_t handle;
> spl_t s;
> int rc;
>
> trace_xn_sys_sem_open(name, oflags, mode, value);
>
> if (name[0] != '/' || name[1] == '\0')
> return ERR_PTR(-EINVAL);
>
> retry_bind:
> rc = xnregistry_bind(&name[1], XN_NONBLOCK, XN_RELATIVE, &handle);
> switch (rc) {
> case 0:
> /* Found */
> if ((oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
> return ERR_PTR(-EEXIST);
>
> xnlock_get_irqsave(&nsem_lock, s);
> u = nsem_hash_search(handle, current->mm);
> if (u) {
> ++u->refs;
> xnlock_put_irqrestore(&nsem_lock, s);
> return u->usem;
> }
> xnlock_put_irqrestore(&nsem_lock, s);
>
> xnlock_get_irqsave(&nklock, s);
> sem = xnregistry_fetch(handle);
> if (sem && sem->magic != COBALT_SEM_MAGIC) {
> xnlock_put_irqrestore(&nklock, s);
> return ERR_PTR(-EINVAL);
> }
>
> if (sem) {
> ++sem->refs;
> xnlock_put_irqrestore(&nklock, s);
> } else {
> xnlock_put_irqrestore(&nklock, s);
> goto retry_bind;
> }
> break;
>
> case -EWOULDBLOCK:
> /* Not found */
> if ((oflags & O_CREAT) == 0)
> return ERR_PTR(-ENOENT);
>
> rc = cobalt_sem_init_inner
> (&name[1], &shadow, SEM_PSHARED, value);
> if (rc < 0) {
> if (rc == -EEXIST)
> goto retry_bind;
> return ERR_PTR(rc);
> }
>
> if (__xn_safe_copy_to_user(ushadow, &shadow, sizeof(shadow))) {
> cobalt_sem_destroy_inner(shadow.handle);
> return ERR_PTR(-EFAULT);
> }
> handle = shadow.handle;
> sem = xnregistry_fetch(handle);
> ++sem->refs;
Why doesn't this lookup and referencing require nklock protection? After
cobalt_sem_init_inner, sem is now globally visible, so someone else
(like a competing nsem_open) could fetch it and reference it, no?
> }
>
> u = xnmalloc(sizeof(*u));
> if (u == NULL)
> return ERR_PTR(-ENOMEM);
>
> u->sem = sem;
> u->mm = current->mm;
> u->usem = ushadow;
> u->refs = 1;
>
> xnlock_get_irqsave(&nsem_lock, s);
> v = nsem_hash_search(handle, current->mm);
> if (v) {
> ++v->refs;
> xnlock_put_irqrestore(&nsem_lock, s);
> xnlock_get_irqsave(&nklock, s);
> --sem->refs;
> xnlock_put_irqrestore(&nklock, s);
>
> if (rc == -EWOULDBLOCK)
> cobalt_sem_destroy_inner(sem->handle);
I'm still not sure I fully understand the reason (and safety) of this
release. We get here when we lost the race to enter handle in the
current process' nsem hash table, right? So v != u, ok. But both v and u
must point to a sem object with the same handle value, otherwise they
wouldn't end up in the same hash table slot. But which of those sem
objects is now registered with the xnregistry? I mean, is this destroy
really working on the right one, the one we created in the current context?
>
> xnfree(u);
> u = v;
> } else {
> nsem_hash_enter(handle, u);
> list_add(&u->link, &cobalt_process_context()->usems);
> xnlock_put_irqrestore(&nsem_lock, s);
> }
>
> trace_xn_sys_sem_open_return(handle);
>
> return u->usem;
> }
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
_______________________________________________
Xenomai mailing list
[email protected]
http://www.xenomai.org/mailman/listinfo/xenomai
