[Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
  Gilles Chanteperdrix (1):
        doc: regenerate

are available in the git repository at:

  git://git.xenomai.org/xenomai-jki.git for-upstream

Unfortunately too late for 2.5.2, fortunately only relevant if something
else went wrong in create_instance (ENOMEM or application error).

Wolfgang Mauerer (1):
      RTDM: Fix potential NULL pointer dereference

 ksrc/skins/rtdm/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>   Gilles Chanteperdrix (1):
>         doc: regenerate
> 
> are available in the git repository at:
> 
>   git://git.xenomai.org/xenomai-jki.git for-upstream
> 
> Unfortunately too late for 2.5.2, fortunately only relevant if something
> else went wrong in create_instance (ENOMEM or application error).
> 
> Wolfgang Mauerer (1):
>       RTDM: Fix potential NULL pointer dereference
> 
>  ksrc/skins/rtdm/core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 

Was not there any plan to remove this code and have a more "posix
compliant" close? I thought this had been merged?

-- 
					    Gilles.

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>   Gilles Chanteperdrix (1):
>>         doc: regenerate
>>
>> are available in the git repository at:
>>
>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>
>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>> else went wrong in create_instance (ENOMEM or application error).
>>
>> Wolfgang Mauerer (1):
>>       RTDM: Fix potential NULL pointer dereference
>>
>>  ksrc/skins/rtdm/core.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
> 
> Was not there any plan to remove this code and have a more "posix
> compliant" close? I thought this had been merged?

Yes. But that requires more work than it would have been appropriate for
2.5.2.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>   Gilles Chanteperdrix (1):
>>>         doc: regenerate
>>>
>>> are available in the git repository at:
>>>
>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>
>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>> else went wrong in create_instance (ENOMEM or application error).
>>>
>>> Wolfgang Mauerer (1):
>>>       RTDM: Fix potential NULL pointer dereference
>>>
>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>> Was not there any plan to remove this code and have a more "posix
>> compliant" close? I thought this had been merged?
> 
> Yes. But that requires more work than it would have been appropriate for
> 2.5.2.

Ok, but could we have it for 2.5.3?


-- 
					    Gilles.

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>   Gilles Chanteperdrix (1):
>>>>         doc: regenerate
>>>>
>>>> are available in the git repository at:
>>>>
>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>
>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>
>>>> Wolfgang Mauerer (1):
>>>>       RTDM: Fix potential NULL pointer dereference
>>>>
>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>> Was not there any plan to remove this code and have a more "posix
>>> compliant" close? I thought this had been merged?
>> Yes. But that requires more work than it would have been appropriate for
>> 2.5.2.
> 
> Ok, but could we have it for 2.5.3?
> 

Will see what I can do (no budget for this, so no promise).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>   Gilles Chanteperdrix (1):
>>>>>         doc: regenerate
>>>>>
>>>>> are available in the git repository at:
>>>>>
>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>
>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>
>>>>> Wolfgang Mauerer (1):
>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>
>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>> Was not there any plan to remove this code and have a more "posix
>>>> compliant" close? I thought this had been merged?
>>> Yes. But that requires more work than it would have been appropriate for
>>> 2.5.2.
>> Ok, but could we have it for 2.5.3?
>>
> 
> Will see what I can do (no budget for this, so no promise).

The patches are floating around for some time (I was in CC of the mail
Philippe sent you about this some time ago), I guess some testing is
needed, this is why you need some time? Could we help with this testing?

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>>   Gilles Chanteperdrix (1):
>>>>>>         doc: regenerate
>>>>>>
>>>>>> are available in the git repository at:
>>>>>>
>>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>>
>>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>>
>>>>>> Wolfgang Mauerer (1):
>>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>>
>>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>
>>>>> Was not there any plan to remove this code and have a more "posix
>>>>> compliant" close? I thought this had been merged?
>>>> Yes. But that requires more work than it would have been appropriate for
>>>> 2.5.2.
>>> Ok, but could we have it for 2.5.3?
>>>
>> Will see what I can do (no budget for this, so no promise).
> 
> The patches are floating around for some time (I was in CC of the mail
> Philippe sent you about this some time ago), I guess some testing is
> needed, this is why you need some time? Could we help with this testing?
> 

Then I totally lost track of this. What series are you referring to?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>>>   Gilles Chanteperdrix (1):
>>>>>>>         doc: regenerate
>>>>>>>
>>>>>>> are available in the git repository at:
>>>>>>>
>>>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>>>
>>>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>>>
>>>>>>> Wolfgang Mauerer (1):
>>>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>>>
>>>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>
>>>>>> Was not there any plan to remove this code and have a more "posix
>>>>>> compliant" close? I thought this had been merged?
>>>>> Yes. But that requires more work than it would have been appropriate for
>>>>> 2.5.2.
>>>> Ok, but could we have it for 2.5.3?
>>>>
>>> Will see what I can do (no budget for this, so no promise).
>> The patches are floating around for some time (I was in CC of the mail
>> Philippe sent you about this some time ago), I guess some testing is
>> needed, this is why you need some time? Could we help with this testing?
>>
> 
> Then I totally lost track of this. What series are you referring to?

A (private) mail entitled "Re: RTDM close() refactoring" from Philippe,
with in fact, a unique patch.

-- 
					    Gilles.

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>>>>   Gilles Chanteperdrix (1):
>>>>>>>>         doc: regenerate
>>>>>>>>
>>>>>>>> are available in the git repository at:
>>>>>>>>
>>>>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>>>>
>>>>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>>>>
>>>>>>>> Wolfgang Mauerer (1):
>>>>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>>>>
>>>>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>>
>>>>>>> Was not there any plan to remove this code and have a more "posix
>>>>>>> compliant" close? I thought this had been merged?
>>>>>> Yes. But that requires more work than it would have been appropriate for
>>>>>> 2.5.2.
>>>>> Ok, but could we have it for 2.5.3?
>>>>>
>>>> Will see what I can do (no budget for this, so no promise).
>>> The patches are floating around for some time (I was in CC of the mail
>>> Philippe sent you about this some time ago), I guess some testing is
>>> needed, this is why you need some time? Could we help with this testing?
>>>
>> Then I totally lost track of this. What series are you referring to?
> 
> A (private) mail entitled "Re: RTDM close() refactoring" from Philippe,
> with in fact, a unique patch.
> 

Ah, now I found it. That discussion ended last September with a "Don't
bother yet" by Philippe as he said you pointed out some remaining issues
/wrt POSIX compliance. So I stopped bothering.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>>>>>   Gilles Chanteperdrix (1):
>>>>>>>>>         doc: regenerate
>>>>>>>>>
>>>>>>>>> are available in the git repository at:
>>>>>>>>>
>>>>>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>>>>>
>>>>>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>>>>>
>>>>>>>>> Wolfgang Mauerer (1):
>>>>>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>>>>>
>>>>>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>>>
>>>>>>>> Was not there any plan to remove this code and have a more "posix
>>>>>>>> compliant" close? I thought this had been merged?
>>>>>>> Yes. But that requires more work than it would have been appropriate for
>>>>>>> 2.5.2.
>>>>>> Ok, but could we have it for 2.5.3?
>>>>>>
>>>>> Will see what I can do (no budget for this, so no promise).
>>>> The patches are floating around for some time (I was in CC of the mail
>>>> Philippe sent you about this some time ago), I guess some testing is
>>>> needed, this is why you need some time? Could we help with this testing?
>>>>
>>> Then I totally lost track of this. What series are you referring to?
>> A (private) mail entitled "Re: RTDM close() refactoring" from Philippe,
>> with in fact, a unique patch.
>>
> 
> Ah, now I found it. That discussion ended last September with a "Don't
> bother yet" by Philippe as he said you pointed out some remaining issues
> /wrt POSIX compliance. So I stopped bothering.

There was a reply "here it is", with a fixed version. Inlined here.

diff --git a/include/asm-generic/wrappers.h b/include/asm-generic/wrappers.h
index 18af70b..71abc5a 100644
--- a/include/asm-generic/wrappers.h
+++ b/include/asm-generic/wrappers.h
@@ -536,4 +536,9 @@ static inline void wrap_proc_dir_entry_owner(struct proc_dir_entry *entry)
 #endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,30) */
 #endif /* CONFIG_PROC_FS */
 
+#ifndef list_first_entry
+#define list_first_entry(ptr, type, member) \
+	list_entry((ptr)->next, type, member)
+#endif
+
 #endif /* _XENO_ASM_GENERIC_WRAPPERS_H */
diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
index ea543c1..d5f997b 100644
--- a/include/rtdm/rtdm_driver.h
+++ b/include/rtdm/rtdm_driver.h
@@ -368,6 +368,7 @@ struct rtdm_operations {
 
 struct rtdm_devctx_reserved {
 	void *owner;
+	struct list_head cleanup;
 };
 
 /**
@@ -549,9 +550,17 @@ static inline void rtdm_context_lock(struct rtdm_dev_context *context)
 	atomic_inc(&context->close_lock_count);
 }
 
+void __rtdm_context_flush(struct rtdm_dev_context *context);
+
 static inline void rtdm_context_unlock(struct rtdm_dev_context *context)
 {
-	atomic_dec(&context->close_lock_count);
+	if (atomic_dec_and_test(&context->close_lock_count))
+		__rtdm_context_flush(context);
+}
+
+static inline void rtdm_context_put(struct rtdm_dev_context *context)
+{
+	rtdm_context_unlock(context);
 }
 
 /* --- clock services --- */
diff --git a/ksrc/skins/rtdm/core.c b/ksrc/skins/rtdm/core.c
index 5843842..c3ff526 100644
--- a/ksrc/skins/rtdm/core.c
+++ b/ksrc/skins/rtdm/core.c
@@ -26,7 +26,7 @@
  * @{
  */
 
-#include <linux/delay.h>
+#include <linux/workqueue.h>
 
 #include <nucleus/pod.h>
 #include <nucleus/ppd.h>
@@ -44,20 +44,25 @@ struct rtdm_fildes fildes_table[RTDM_FD_MAX] =
 static unsigned long used_fildes[FD_BITMAP_SIZE];
 int open_fildes;	/* number of used descriptors */
 
+static DECLARE_WORK_FUNC(close_callback);
+static DECLARE_WORK_NODATA(close_work, close_callback);
+static LIST_HEAD(cleanup_queue);
+
 xntbase_t *rtdm_tbase;
 EXPORT_SYMBOL(rtdm_tbase);
 
 DEFINE_XNLOCK(rt_fildes_lock);
 
 /**
- * @brief Resolve file descriptor to device context
+ * @brief Retrieve and lock a device context
  *
  * @param[in] fd File descriptor
  *
  * @return Pointer to associated device context, or NULL on error
  *
- * @note The device context has to be unlocked using rtdm_context_unlock()
- * when it is no longer referenced.
+ * @note The device context has to be unlocked using
+ * rtdm_context_put() or rtdm_context_unlock() when it is no longer
+ * referenced.
  *
  * Environments:
  *
@@ -155,7 +160,7 @@ static int create_instance(struct rtdm_device *device,
 
 	context->fd = fd;
 	context->ops = &device->ops;
-	atomic_set(&context->close_lock_count, 0);
+	atomic_set(&context->close_lock_count, 1);
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	ppd = xnshadow_ppd_get(__rtdm_muxid);
@@ -163,24 +168,32 @@ static int create_instance(struct rtdm_device *device,
 
 	context->reserved.owner =
 	    ppd ? container_of(ppd, struct rtdm_process, ppd) : NULL;
+	INIT_LIST_HEAD(&context->reserved.cleanup);
 
 	return 0;
 }
 
-/* call with rt_fildes_lock acquired - will release it */
-static void cleanup_instance(struct rtdm_device *device,
-			     struct rtdm_dev_context *context,
-			     struct rtdm_fildes *fildes, int nrt_mem, spl_t s)
+/* must be called with rt_fildes_lock acquired */
+static void cleanup_fildes(struct rtdm_fildes *fildes)
 {
-	if (fildes) {
-		clear_bit((fildes - fildes_table), used_fildes);
-		fildes->context = NULL;
-		open_fildes--;
-	}
+	int fd = fildes - fildes_table;
 
-	xnlock_put_irqrestore(&rt_fildes_lock, s);
+	clear_bit(fd, used_fildes);
+	fildes->context->fd = -1;
+	fildes->context = NULL;
+	open_fildes--;
 
+	trace_mark(xn_rtdm, cleanup_fildes, "fd %d", fd);
+}
+
+/* must be called lock free */
+static void cleanup_context(struct rtdm_device *device,
+			    struct rtdm_dev_context *context,
+			    int nrt_mem)
+{
 	if (context) {
+		int fd = context->fd;
+
 		if (device->reserved.exclusive_context)
 			context->device = NULL;
 		else {
@@ -189,11 +202,26 @@ static void cleanup_instance(struct rtdm_device *device,
 			else
 				xnfree(context);
 		}
+
+		trace_mark(xn_rtdm, cleanup_context, "fd %d", fd);
 	}
 
 	rtdm_dereference_device(device);
 }
 
+/* call with rt_fildes_lock acquired - will release it */
+static void cleanup_instance(struct rtdm_device *device,
+			     struct rtdm_dev_context *context,
+			     struct rtdm_fildes *fildes, int nrt_mem, spl_t s)
+{
+	if (fildes)
+		cleanup_fildes(fildes);
+
+	xnlock_put_irqrestore(&rt_fildes_lock, s);
+
+	cleanup_context(device, context, nrt_mem);
+}
+
 int __rt_dev_open(rtdm_user_info_t *user_info, const char *path, int oflag)
 {
 	struct rtdm_device *device;
@@ -298,82 +326,144 @@ err_out:
 
 EXPORT_SYMBOL(__rt_dev_socket);
 
-int __rt_dev_close(rtdm_user_info_t *user_info, int fd)
+static DECLARE_WORK_FUNC(close_callback)
 {
 	struct rtdm_dev_context *context;
 	spl_t s;
-	int ret;
-	int nrt_mode = !rtdm_in_rt_context();
 
-	trace_mark(xn_rtdm, close, "user_info %p fd %d", user_info, fd);
-
-	ret = -EBADF;
-	if (unlikely((unsigned int)fd >= RTDM_FD_MAX))
-		goto err_out;
-
-again:
 	xnlock_get_irqsave(&rt_fildes_lock, s);
 
-	context = fildes_table[fd].context;
-
-	if (unlikely(!context)) {
+	while (!list_empty(&cleanup_queue)) {
+		context = list_first_entry(&cleanup_queue,
+					   struct rtdm_dev_context, reserved.cleanup);
+		list_del(&context->reserved.cleanup);
 		xnlock_put_irqrestore(&rt_fildes_lock, s);
-		goto err_out;	/* -EBADF */
+		context->ops->close_nrt(context, NULL);
+		cleanup_context(context->device, context,
+				test_bit(RTDM_CREATED_IN_NRT,
+					 &context->context_flags));
+		xnlock_get_irqsave(&rt_fildes_lock, s);
 	}
 
-	set_bit(RTDM_CLOSING, &context->context_flags);
-	rtdm_context_lock(context);
-
 	xnlock_put_irqrestore(&rt_fildes_lock, s);
+}
 
-	if (nrt_mode)
-		ret = context->ops->close_nrt(context, user_info);
-	else {
-		/* Avoid asymmetric close context by switching to nrt. */
-		if (unlikely(
-		    test_bit(RTDM_CREATED_IN_NRT, &context->context_flags))) {
-			ret = -ENOSYS;
-			goto unlock_out;
+void rtdm_apc_handler(void *cookie)
+{
+	schedule_work(&close_work);
+}
+
+void __rtdm_context_flush(struct rtdm_dev_context *context)
+{
+	spl_t s;
+
+	xnlock_get_irqsave(&rt_fildes_lock, s);
+	
+	/*
+	 * The only valid context locking construct which driver may
+	 * use is:
+	 *
+	 * context = rtdm_context_get(fd)
+	 *     ...
+	 *     [ rtdm_context_lock(context);
+	 *       ... (optional nested locking)
+	 *       rtdm_context_unlock(context); ]
+	 *     ...
+	 * rtdm_context_put/unlock(context);
+	 *
+	 * Anything different, and particularly calling
+	 * rtdm_lock_context() on device contexts NOT initially
+	 * obtained from rtdm_context_get() will open a race window
+	 * which may cause access to stale memory.
+	 *
+	 * The following assertion is an attempt to detect such misuse
+	 * of the locking interface.
+	 */
+	XENO_ASSERT(RTDM,
+		    atomic_read(&context->close_lock_count),
+		    goto unlock_out);
+	/*
+	 * __rt_dev_close() already released the file descriptor. We
+	 * just need to discard the context struct.
+	 */
+	if (unlikely(rtdm_in_rt_context())) {
+		if (unlikely(context->ops->close_rt)) {
+			xnlock_put_irqrestore(&rt_fildes_lock, s);
+			context->ops->close_rt(context, NULL);
+			goto cleanup;
 		}
-		ret = context->ops->close_rt(context, user_info);
+		list_add_tail(&context->reserved.cleanup, &cleanup_queue);
+		rthal_apc_schedule(rtdm_apc);
+		goto unlock_out;
 	}
 
-	XENO_ASSERT(RTDM, !rthal_local_irq_disabled(),
-		    rthal_local_irq_enable(););
+	/*
+	 * No reference is pending, and we are running in NRT mode, we
+	 * may run the close_nrt() handler and flush the context
+	 * immediately.
+	 */
+	xnlock_put_irqrestore(&rt_fildes_lock, s);
 
-	if (unlikely(ret == -EAGAIN) && nrt_mode) {
-		rtdm_context_unlock(context);
-		msleep(CLOSURE_RETRY_PERIOD);
-		goto again;
-	} else if (unlikely(ret < 0))
-		goto unlock_out;
+	XENO_BUGON(RTDM, in_atomic());
+
+	context->ops->close_nrt(context, NULL);
+
+cleanup:
+	cleanup_context(context->device, context,
+			test_bit(RTDM_CREATED_IN_NRT,
+				 &context->context_flags));
+	return;
+
+unlock_out:
+	xnlock_put_irqrestore(&rt_fildes_lock, s);
+}
+
+EXPORT_SYMBOL(__rtdm_context_flush);
+
+int __rt_dev_close(rtdm_user_info_t *user_info, int fd)
+{
+	struct rtdm_dev_context *context;
+	struct rtdm_fildes *fildes;
+	int ret;
+	spl_t s;
+
+	trace_mark(xn_rtdm, close, "user_info %p fd %d", user_info, fd);
+
+	ret = -EBADF;
+	if (unlikely((unsigned int)fd >= RTDM_FD_MAX))
+		return ret;
 
 	xnlock_get_irqsave(&rt_fildes_lock, s);
 
-	if (unlikely(atomic_read(&context->close_lock_count) > 1)) {
-		xnlock_put_irqrestore(&rt_fildes_lock, s);
+	fildes = fildes_table + fd;
+	context = fildes->context;
+	if (unlikely(context == NULL))
+		goto unlock_out;
+
+	if (test_and_set_bit(RTDM_CLOSING, &context->context_flags))
+		goto unlock_out; /* close() in progress */
 
-		if (!nrt_mode) {
-			ret = -EAGAIN;
+	/* Avoid asymmetric close context by switching to nrt. */
+	if (unlikely(rtdm_in_rt_context() &&
+		     test_bit(RTDM_CREATED_IN_NRT, &context->context_flags))) {
+			ret = -ENOSYS;
 			goto unlock_out;
-		}
-		rtdm_context_unlock(context);
-		msleep(CLOSURE_RETRY_PERIOD);
-		goto again;
 	}
 
-	cleanup_instance(context->device, context, &fildes_table[fd],
-			 test_bit(RTDM_CREATED_IN_NRT, &context->context_flags),
-			 s);
+	cleanup_fildes(fildes);
+
+	xnlock_put_irqrestore(&rt_fildes_lock, s);
+
+	/* Release the reference obtained in open() */
+	rtdm_context_unlock(context);
 
 	trace_mark(xn_rtdm, fd_closed, "fd %d", fd);
 
-	return ret;
+	return 0;
 
 unlock_out:
-	rtdm_context_unlock(context);
+	xnlock_put_irqrestore(&rt_fildes_lock, s);
 
-err_out:
 	return ret;
 }
 
@@ -586,8 +676,12 @@ EXPORT_SYMBOL(rtdm_select_bind);
  *
  * @param[in] context Device context
  *
- * @note rtdm_context_get() automatically increments the lock counter. You
- * only need to call this function in special scenrios.
+ * @note rtdm_context_get() automatically increments the lock counter.
+ * WARNING: You may call rtdm_context_lock() only to hold an
+ * additional reference on a context initially locked by
+ * rtdm_context_get(). Using rtdm_context_lock() on device contexts
+ * NOT obtained this way may lead to access stale memory in some
+ * circumstances.
  *
  * Environments:
  *
@@ -607,8 +701,8 @@ void rtdm_context_lock(struct rtdm_dev_context *context);
  *
  * @param[in] context Device context
  *
- * @note Every successful call to rtdm_context_get() must be matched by a
- * rtdm_context_unlock() invocation.
+ * @note Every successful call to rtdm_context_lock() must be matched
+ * by a rtdm_context_unlock() invocation.
  *
  * Environments:
  *
@@ -624,6 +718,26 @@ void rtdm_context_lock(struct rtdm_dev_context *context);
 void rtdm_context_unlock(struct rtdm_dev_context *context);
 
 /**
+ * @brief Release a device context
+ *
+ * @param[in] context Device context
+ *
+ * @note This routine is an alias for rtdm_context_unlock().
+ *
+ * Environments:
+ *
+ * This service can be called from:
+ *
+ * - Kernel module initialization/cleanup code
+ * - Interrupt service routine
+ * - Kernel-based task
+ * - User-space task (RT, non-RT)
+ *
+ * Rescheduling: never.
+ */
+void rtdm_context_put(struct rtdm_dev_context *context);
+
+/**
  * @brief Open a device
  *
  * Refer to rt_dev_open() for parameters and return values
diff --git a/ksrc/skins/rtdm/device.c b/ksrc/skins/rtdm/device.c
index e927ce7..9838622 100644
--- a/ksrc/skins/rtdm/device.c
+++ b/ksrc/skins/rtdm/device.c
@@ -55,6 +55,7 @@ MODULE_PARM_DESC(protocol_hashtab_size,
 
 struct list_head *rtdm_named_devices;	/* hash table */
 struct list_head *rtdm_protocol_devices;	/* hash table */
+int rtdm_apc;
 static int name_hashkey_mask;
 static int proto_hashkey_mask;
 
@@ -238,12 +239,12 @@ int rtdm_dev_register(struct rtdm_device *device)
 
 	/* Sanity check: non-RT close handler?
 	 * (Always required for forced cleanup) */
-	if (!device->ops.close_nrt) {
+	if (device->ops.close_nrt == NULL) {
 		xnlogerr("RTDM: missing non-RT close handler\n");
 		return -EINVAL;
 	}
-	if (!device->ops.close_rt)
-		device->ops.close_rt = (void *)rtdm_no_support;
+	if (device->ops.close_rt)
+		xnlogerr("RTDM: RT close handler is deprecated - convert to NRT\n");
 
 	SET_DEFAULT_OP_IF_NULL(device->ops, ioctl);
 	SET_DEFAULT_OP_IF_NULL(device->ops, read);
@@ -448,6 +449,10 @@ int __init rtdm_dev_init(void)
 {
 	int i;
 
+	rtdm_apc = rthal_apc_alloc("rtdm_handler", rtdm_apc_handler, NULL);
+	if (rtdm_apc < 0)
+		return rtdm_apc;
+
 	name_hashkey_mask = devname_hashtab_size - 1;
 	proto_hashkey_mask = protocol_hashtab_size - 1;
 	if (((devname_hashtab_size & name_hashkey_mask) != 0) ||
@@ -477,4 +482,14 @@ int __init rtdm_dev_init(void)
 	return 0;
 }
 
+void __exit rtdm_dev_cleanup(void)
+{
+	rthal_apc_free(rtdm_apc);
+	rtdm_apc = -1;
+	xnarch_memory_barrier();
+	flush_scheduled_work();
+	kfree(rtdm_named_devices);
+	kfree(rtdm_protocol_devices);
+}
+
 /*@}*/
diff --git a/ksrc/skins/rtdm/internal.h b/ksrc/skins/rtdm/internal.h
index 64e5b47..c729e19 100644
--- a/ksrc/skins/rtdm/internal.h
+++ b/ksrc/skins/rtdm/internal.h
@@ -58,6 +58,7 @@ extern unsigned int protocol_hashtab_size;
 extern struct list_head *rtdm_named_devices;
 extern struct list_head *rtdm_protocol_devices;
 extern struct proc_dir_entry *rtdm_proc_root;
+extern int rtdm_apc;
 
 #ifdef MODULE
 #define rtdm_initialised 1
@@ -76,15 +77,11 @@ static inline void rtdm_dereference_device(struct rtdm_device *device)
 }
 
 int __init rtdm_dev_init(void);
-
-static inline void rtdm_dev_cleanup(void)
-{
-	kfree(rtdm_named_devices);
-	kfree(rtdm_protocol_devices);
-}
+void __exit rtdm_dev_cleanup(void);
 
 int rtdm_proc_register_device(struct rtdm_device *device);
 int __init rtdm_proc_init(void);
 void rtdm_proc_cleanup(void);
+void rtdm_apc_handler(void *cookie);
 
 #endif /* _RTDM_INTERNAL_H */

-- 
					    Gilles.

Re: [Xenomai-core] [git pull] RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> The following changes since commit 6b3e8f2e5c69397814cb2f4029cdfbaff16e113e:
>>>>>>>>>   Gilles Chanteperdrix (1):
>>>>>>>>>         doc: regenerate
>>>>>>>>>
>>>>>>>>> are available in the git repository at:
>>>>>>>>>
>>>>>>>>>   git://git.xenomai.org/xenomai-jki.git for-upstream
>>>>>>>>>
>>>>>>>>> Unfortunately too late for 2.5.2, fortunately only relevant if something
>>>>>>>>> else went wrong in create_instance (ENOMEM or application error).
>>>>>>>>>
>>>>>>>>> Wolfgang Mauerer (1):
>>>>>>>>>       RTDM: Fix potential NULL pointer dereference
>>>>>>>>>
>>>>>>>>>  ksrc/skins/rtdm/core.c |    2 +-
>>>>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>>>
>>>>>>>> Was not there any plan to remove this code and have a more "posix
>>>>>>>> compliant" close? I thought this had been merged?
>>>>>>> Yes. But that requires more work than it would have been appropriate for
>>>>>>> 2.5.2.
>>>>>> Ok, but could we have it for 2.5.3?
>>>>>>
>>>>> Will see what I can do (no budget for this, so no promise).
>>>> The patches are floating around for some time (I was in CC of the mail
>>>> Philippe sent you about this some time ago), I guess some testing is
>>>> needed, this is why you need some time? Could we help with this testing?
>>>>
>>> Then I totally lost track of this. What series are you referring to?
>> A (private) mail entitled "Re: RTDM close() refactoring" from Philippe,
>> with in fact, a unique patch.
>>
> 
> Ah, now I found it. That discussion ended last September with a "Don't
> bother yet" by Philippe as he said you pointed out some remaining issues
> /wrt POSIX compliance. So I stopped bothering.

Oh, no, it stopped with an update I wanted to look at. OK, that got lost
on my side. (Better to have that on the list instead of my overfull
inbox next time.)

Will see if it still applies.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux