[PATCH 0/4] IPC: CRIU enhancements fixes and cleanup

[PATCH 0/4] IPC: CRIU enhancements fixes and cleanup

[Stanislav Kinsbursky]

This patch set simplifies message queue copy feature and clean up it's
implementation.
It also adds some debug and fixes an issue, when copy_msg() fails. In this
case error have to returned instead of breaking messages loop, because error
message pointer is interpreted as -EAGAIN in current implemetation of further
message handling.

The following series implements...

---

Stanislav Kinsbursky (4):
      ipc: simplify free_copy() call
      ipc: convert prepare_copy() from macro to function
      ipc: simplify message copying
      ipc: add more comments to message copying related code


 ipc/msg.c     |   55 +++++++++++++++++++++++++++++++++----------------------
 ipc/msgutil.c |    5 +++++
 2 files changed, 38 insertions(+), 22 deletions(-)

[PATCH 1/4] ipc: simplify free_copy() call

[Stanislav Kinsbursky]

Passing and checking of msgflg to free_copy() is redundant.
This patch sets copy to NULL on declaration instead and checks for non-NULL in
free_copy().

Note: in case of copy allocation failure, error is returned immediately. So
no need to check for IS_ERR() in free_copy().

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 ipc/msg.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index a0b0224..f1070c3 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -797,15 +797,17 @@ static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
 	return copy;
 }
 
-static inline void free_copy(int msgflg, struct msg_msg *copy)
+static inline void free_copy(struct msg_msg *copy)
 {
-	if (msgflg & MSG_COPY)
+	if (copy)
 		free_msg(copy);
 }
 #else
-#define free_copy(msgflg, copy)				do {} while (0)
 #define prepare_copy(buf, sz, msgflg, msgtyp, copy_nr)	ERR_PTR(-ENOSYS)
 #define fill_copy(copy_nr, msg_nr, msg, copy)		NULL
+static inline void free_copy(struct msg_msg *copy)
+{
+}
 #endif
 
 long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
@@ -816,7 +818,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	struct msg_msg *msg;
 	int mode;
 	struct ipc_namespace *ns;
-	struct msg_msg *copy;
+	struct msg_msg *copy = NULL;
 	unsigned long __maybe_unused copy_number;
 
 	if (msqid < 0 || (long) bufsz < 0)
@@ -831,7 +833,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 
 	msq = msg_lock_check(ns, msqid);
 	if (IS_ERR(msq)) {
-		free_copy(msgflg, copy);
+		free_copy(copy);
 		return PTR_ERR(msq);
 	}
 
@@ -964,7 +966,7 @@ out_unlock:
 		}
 	}
 	if (IS_ERR(msg)) {
-		free_copy(msgflg, copy);
+		free_copy(copy);
 		return PTR_ERR(msg);
 	}
 

[PATCH 2/4] ipc: convert prepare_copy() from macro to function

[Stanislav Kinsbursky]

This code works if CONFIG_CHECKPOINT_RESTORE is disabled.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 ipc/msg.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index f1070c3..ad194f8 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -803,8 +803,15 @@ static inline void free_copy(struct msg_msg *copy)
 		free_msg(copy);
 }
 #else
-#define prepare_copy(buf, sz, msgflg, msgtyp, copy_nr)	ERR_PTR(-ENOSYS)
 #define fill_copy(copy_nr, msg_nr, msg, copy)		NULL
+
+static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
+					   int msgflg, long *msgtyp,
+					   unsigned long *copy_number)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline void free_copy(struct msg_msg *copy)
 {
 }
@@ -819,7 +826,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	int mode;
 	struct ipc_namespace *ns;
 	struct msg_msg *copy = NULL;
-	unsigned long __maybe_unused copy_number;
+	unsigned long __maybe_unused copy_number = 0;
 
 	if (msqid < 0 || (long) bufsz < 0)
 		return -EINVAL;

[PATCH 3/4] ipc: simplify message copying

[Stanislav Kinsbursky]

This patch removed redundant and confusing fill_copy(). It also adds
copy_msg() check for error. In this case exit from the function have to be
done instead of break, because further code interprets any error as EAGAIN.

It also defines copy_msg() for the case when CONFIG_CHECKPOINT_RESTORE is
disabled.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 ipc/msg.c     |   24 +++++++++---------------
 ipc/msgutil.c |    5 +++++
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index ad194f8..5e317fe 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -770,16 +770,6 @@ static long do_msg_fill(void __user *dest, struct msg_msg *msg, size_t bufsz)
 }
 
 #ifdef CONFIG_CHECKPOINT_RESTORE
-static inline struct msg_msg *fill_copy(unsigned long copy_nr,
-					unsigned long msg_nr,
-					struct msg_msg *msg,
-					struct msg_msg *copy)
-{
-	if (copy_nr == msg_nr)
-		return copy_msg(msg, copy);
-	return NULL;
-}
-
 static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
 					   int msgflg, long *msgtyp,
 					   unsigned long *copy_number)
@@ -803,8 +793,6 @@ static inline void free_copy(struct msg_msg *copy)
 		free_msg(copy);
 }
 #else
-#define fill_copy(copy_nr, msg_nr, msg, copy)		NULL
-
 static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
 					   int msgflg, long *msgtyp,
 					   unsigned long *copy_number)
@@ -868,10 +856,16 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 						walk_msg->m_type != 1) {
 					msgtyp = walk_msg->m_type - 1;
 				} else if (msgflg & MSG_COPY) {
-					msg = fill_copy(copy_number, msg_counter,
-							walk_msg, copy);
-					if (msg)
+					if (copy_number == msg_counter) {
+						/*
+						 * Found requested message.
+						 * Copy it.
+						 */
+						msg = copy_msg(msg, copy);
+						if (IS_ERR(msg))
+							goto out_unlock;
 						break;
+					}
 				} else
 					break;
 				msg_counter++;
diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index b281f5c..4168bb8 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -138,6 +138,11 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 
 	return dst;
 }
+#else
+struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
+{
+	return ERR_PTR(-ENOSYS);
+}
 #endif
 int store_msg(void __user *dest, struct msg_msg *msg, int len)
 {

[PATCH 4/4] ipc: add more comments to message copying related code

[Stanislav Kinsbursky]

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 ipc/msg.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 5e317fe..4a4725c 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -770,6 +770,10 @@ static long do_msg_fill(void __user *dest, struct msg_msg *msg, size_t bufsz)
 }
 
 #ifdef CONFIG_CHECKPOINT_RESTORE
+/*
+ * This function creates new kernel message structure, large enough to store
+ * bufsz message bytes.
+ */
 static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
 					   int msgflg, long *msgtyp,
 					   unsigned long *copy_number)
@@ -881,6 +885,10 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 				msg = ERR_PTR(-E2BIG);
 				goto out_unlock;
 			}
+			/*
+			 * If we are copying, then do not unlink message and do
+			 * not update queue parameters.
+			 */
 			if (msgflg & MSG_COPY)
 				goto out_unlock;
 			list_del(&msg->m_list);

Re: [PATCH 2/4] ipc: convert prepare_copy() from macro to function

[Andrew Morton]

On Wed, 07 Nov 2012 13:05:00 +0300
Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:

> This code works if CONFIG_CHECKPOINT_RESTORE is disabled.
> 
> ...
>
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -803,8 +803,15 @@ static inline void free_copy(struct msg_msg *copy)
>  		free_msg(copy);
>  }
>  #else
> -#define prepare_copy(buf, sz, msgflg, msgtyp, copy_nr)	ERR_PTR(-ENOSYS)
>  #define fill_copy(copy_nr, msg_nr, msg, copy)		NULL
> +
> +static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
> +					   int msgflg, long *msgtyp,
> +					   unsigned long *copy_number)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
>  static inline void free_copy(struct msg_msg *copy)
>  {
>  }
> @@ -819,7 +826,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
>  	int mode;
>  	struct ipc_namespace *ns;
>  	struct msg_msg *copy = NULL;
> -	unsigned long __maybe_unused copy_number;
> +	unsigned long __maybe_unused copy_number = 0;

The __maybe_unused here makes no sense.  I'll remove it.

>  
>  	if (msqid < 0 || (long) bufsz < 0)
>  		return -EINVAL;