[PATCH] convert dm_ulog_request data to little endian

[PATCH] convert dm_ulog_request data to little endian

[Dongmao Zhang]

the dm_ulog_request might be little endian or big endian depending on
the architecture. This is not right. This patch is to convert
dm_ulog_request to little endian.

I met a bug when running cmirrord on s390 linux

Signed-off-by: Dongmao Zhang <dmzhang@suse.com>
---
 drivers/md/dm-log-userspace-transfer.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index b428c0a..cddef2f 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -53,6 +53,29 @@ struct receiving_pkg {
 static DEFINE_SPINLOCK(receiving_list_lock);
 static struct list_head receiving_list;
 
+static void cpu_to_network(struct dm_ulog_request *tfr)
+{
+	if (tfr == NULL)
+		return;
+	tfr->luid = cpu_to_le64(tfr->luid);
+	tfr->version =  cpu_to_le32(tfr->version);
+	tfr->seq = cpu_to_le32(tfr->seq);
+	tfr->request_type = cpu_to_le32(tfr->request_type);
+	tfr->data_size = cpu_to_le32(tfr->data_size);
+}
+
+static void network_to_cpu(struct dm_ulog_request *tfr)
+{
+	if (tfr == NULL)
+		return;
+	tfr->luid = le64_to_cpu(tfr->luid);
+	tfr->version =  le32_to_cpu(tfr->version);
+	tfr->seq = le32_to_cpu(tfr->seq);
+	tfr->request_type = le32_to_cpu(tfr->request_type);
+	tfr->data_size = le32_to_cpu(tfr->data_size);
+	tfr->error = le32_to_cpu(tfr->error);
+}
+
 static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
 {
 	int r;
@@ -66,6 +89,7 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
 	msg->seq = tfr->seq;
 	msg->len = sizeof(struct dm_ulog_request) + tfr->data_size;
 
+	cpu_to_network(tfr);
 	r = cn_netlink_send(msg, 0, 0, gfp_any());
 
 	return r;
@@ -81,8 +105,11 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
  */
 static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
 {
-	uint32_t rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
 	struct receiving_pkg *pkg;
+	uint32_t rtn_seq;
+
+	network_to_cpu(tfr);
+	rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
 
 	/*
 	 * The 'receiving_pkg' entries in this list are statically
@@ -148,6 +175,8 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	spin_unlock(&receiving_list_lock);
 }
 
+
+
 /**
  * dm_consult_userspace
  * @uuid: log's universal unique identifier (must be DM_UUID_LEN in size)
-- 
1.8.4.5

Re: convert dm_ulog_request data to little endian

[Mike Snitzer]

On Thu, Aug 07 2014 at  8:56pm -0400,
Dongmao Zhang <dmzhang@suse.com> wrote:

> the dm_ulog_request might be little endian or big endian depending on
> the architecture. This is not right. This patch is to convert
> dm_ulog_request to little endian.
> 
> I met a bug when running cmirrord on s390 linux
> 
> Signed-off-by: Dongmao Zhang <dmzhang@suse.com>
> ---
>  drivers/md/dm-log-userspace-transfer.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
> index b428c0a..cddef2f 100644
> --- a/drivers/md/dm-log-userspace-transfer.c
> +++ b/drivers/md/dm-log-userspace-transfer.c
> @@ -53,6 +53,29 @@ struct receiving_pkg {
>  static DEFINE_SPINLOCK(receiving_list_lock);
>  static struct list_head receiving_list;
>  
> +static void cpu_to_network(struct dm_ulog_request *tfr)
> +{
> +	if (tfr == NULL)
> +		return;
> +	tfr->luid = cpu_to_le64(tfr->luid);
> +	tfr->version =  cpu_to_le32(tfr->version);
> +	tfr->seq = cpu_to_le32(tfr->seq);
> +	tfr->request_type = cpu_to_le32(tfr->request_type);
> +	tfr->data_size = cpu_to_le32(tfr->data_size);
> +}
> +
> +static void network_to_cpu(struct dm_ulog_request *tfr)
> +{
> +	if (tfr == NULL)
> +		return;
> +	tfr->luid = le64_to_cpu(tfr->luid);
> +	tfr->version =  le32_to_cpu(tfr->version);
> +	tfr->seq = le32_to_cpu(tfr->seq);
> +	tfr->request_type = le32_to_cpu(tfr->request_type);
> +	tfr->data_size = le32_to_cpu(tfr->data_size);
> +	tfr->error = le32_to_cpu(tfr->error);
> +}
> +
>  static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
>  {
>  	int r;
> @@ -66,6 +89,7 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
>  	msg->seq = tfr->seq;
>  	msg->len = sizeof(struct dm_ulog_request) + tfr->data_size;
>  
> +	cpu_to_network(tfr);
>  	r = cn_netlink_send(msg, 0, 0, gfp_any());
>  
>  	return r;
> @@ -81,8 +105,11 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
>   */
>  static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
>  {
> -	uint32_t rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
>  	struct receiving_pkg *pkg;
> +	uint32_t rtn_seq;
> +
> +	network_to_cpu(tfr);
> +	rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
>  
>  	/*
>  	 * The 'receiving_pkg' entries in this list are statically
> @@ -148,6 +175,8 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
>  	spin_unlock(&receiving_list_lock);
>  }
>  
> +
> +
>  /**
>   * dm_consult_userspace
>   * @uuid: log's universal unique identifier (must be DM_UUID_LEN in size)

Other than this last hunk (extra whitespace) this patch looks fine to me.

Jon/Mikulas/Alasdair: could you review this too?  If you guys agree I'll
get it staged for 3.17 (and mark it for stable).

Thanks,
Mike

Re: convert dm_ulog_request data to little endian

[Mikulas Patocka]

On Sun, 10 Aug 2014, Mike Snitzer wrote:

> On Thu, Aug 07 2014 at  8:56pm -0400,
> Dongmao Zhang <dmzhang@suse.com> wrote:
> 
> > the dm_ulog_request might be little endian or big endian depending on
> > the architecture. This is not right. This patch is to convert
> > dm_ulog_request to little endian.
> > 
> > I met a bug when running cmirrord on s390 linux
> > 
> > Signed-off-by: Dongmao Zhang <dmzhang@suse.com>
> > ---
> >  drivers/md/dm-log-userspace-transfer.c | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
> > index b428c0a..cddef2f 100644
> > --- a/drivers/md/dm-log-userspace-transfer.c
> > +++ b/drivers/md/dm-log-userspace-transfer.c
> > @@ -53,6 +53,29 @@ struct receiving_pkg {
> >  static DEFINE_SPINLOCK(receiving_list_lock);
> >  static struct list_head receiving_list;
> >  
> > +static void cpu_to_network(struct dm_ulog_request *tfr)
> > +{
> > +	if (tfr == NULL)
> > +		return;
> > +	tfr->luid = cpu_to_le64(tfr->luid);
> > +	tfr->version =  cpu_to_le32(tfr->version);
> > +	tfr->seq = cpu_to_le32(tfr->seq);
> > +	tfr->request_type = cpu_to_le32(tfr->request_type);
> > +	tfr->data_size = cpu_to_le32(tfr->data_size);
> > +}
> > +
> > +static void network_to_cpu(struct dm_ulog_request *tfr)
> > +{
> > +	if (tfr == NULL)
> > +		return;
> > +	tfr->luid = le64_to_cpu(tfr->luid);
> > +	tfr->version =  le32_to_cpu(tfr->version);
> > +	tfr->seq = le32_to_cpu(tfr->seq);
> > +	tfr->request_type = le32_to_cpu(tfr->request_type);
> > +	tfr->data_size = le32_to_cpu(tfr->data_size);
> > +	tfr->error = le32_to_cpu(tfr->error);
> > +}
> > +
> >  static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
> >  {
> >  	int r;
> > @@ -66,6 +89,7 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
> >  	msg->seq = tfr->seq;
> >  	msg->len = sizeof(struct dm_ulog_request) + tfr->data_size;
> >  
> > +	cpu_to_network(tfr);
> >  	r = cn_netlink_send(msg, 0, 0, gfp_any());
> >  
> >  	return r;
> > @@ -81,8 +105,11 @@ static int dm_ulog_sendto_server(struct dm_ulog_request *tfr)
> >   */
> >  static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
> >  {
> > -	uint32_t rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
> >  	struct receiving_pkg *pkg;
> > +	uint32_t rtn_seq;
> > +
> > +	network_to_cpu(tfr);
> > +	rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
> >  
> >  	/*
> >  	 * The 'receiving_pkg' entries in this list are statically
> > @@ -148,6 +175,8 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
> >  	spin_unlock(&receiving_list_lock);
> >  }
> >  
> > +
> > +
> >  /**
> >   * dm_consult_userspace
> >   * @uuid: log's universal unique identifier (must be DM_UUID_LEN in size)
> 
> Other than this last hunk (extra whitespace) this patch looks fine to me.
> 
> Jon/Mikulas/Alasdair: could you review this too?  If you guys agree I'll
> get it staged for 3.17 (and mark it for stable).
> 
> Thanks,
> Mike

Hi

Red Hat is shipping RHEL7 on s390x and ppc64, so why haven't QA caught 
this?

Is there a possibility that this patch fixes one case and breaks some 
other case?

There is a risk that we fix one userspace program that assumes messages to 
be in little endian and break another userspace program that assumes that 
the messages are in native endian - to avoid this risk, it may be better 
to fix the userspace program and leave the kernel unchanged.

I think you need to find the piece of userspace code that processes the 
information on the netlink socket and explain why it needs the information 
in little endian. (and you also need to show that there is no userspace 
code that expects the data in native endian).

Mikulas

Re: convert dm_ulog_request data to little endian

[Brassow Jonathan]

On Aug 10, 2014, at 10:15 PM, Mike Snitzer wrote:

> 
> Other than this last hunk (extra whitespace) this patch looks fine to me.
> 
> Jon/Mikulas/Alasdair: could you review this too?  If you guys agree I'll
> get it staged for 3.17 (and mark it for stable).

please do not stage.  I think we will fix this in userspace.

 brassow

Re: convert dm_ulog_request data to little endian

[zhangdongmao]

于 2014年08月21日 14:00, Brassow Jonathan 写道:
> On Aug 10, 2014, at 10:15 PM, Mike Snitzer wrote:
>
>> Other than this last hunk (extra whitespace) this patch looks fine to me.
>>
>> Jon/Mikulas/Alasdair: could you review this too?  If you guys agree I'll
>> get it staged for 3.17 (and mark it for stable).
> please do not stage.  I think we will fix this in userspace.
>
>   brassow

Yes, I think so, after comments from brassow,
this could be fixed in userspace. I have a new patch for cmirrord.
Only wait timeslot to test it on S390 and mixed arch.



> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] convert dm_ulog_request data to little endian

[Brassow Jonathan]

On Aug 7, 2014, at 7:56 PM, Dongmao Zhang wrote:

> the dm_ulog_request might be little endian or big endian depending on
> the architecture. This is not right. This patch is to convert
> dm_ulog_request to little endian.
> 
> I met a bug when running cmirrord on s390 linux

This seems odd to me.  I don't understand why you would get a bug.  Do you have a mixed-architecture cluster?  Some x86-64 and the s390?  Otherwise, all operations should be in a format that is understood (unless there is a bug).

Also, you don't really need to translate the structure just to go to userspace.  Only the process that communicates between (potentially different architectures of) machines needs to do that.  That process is cmirrord and the code that should translate for that daemon is in lvm2/daemons/cmirrord/compat.c.

There could be a bug, but unless I'm not understanding right, we shouldn't need to switch endian in the kernel.

 brassow

Re: [PATCH] convert dm_ulog_request data to little endian

[zhangdongmao]

于 2014年08月20日 08:58, Brassow Jonathan 写道:
> This seems odd to me.  I don't understand why you would get a bug.  Do you have a mixed-architecture cluster?  Some x86-64 and the s390?  Otherwise, all operations should be in a format that is understood (unless there is a bug).
>
> Also, you don't really need to translate the structure just to go to userspace.  Only the process that communicates between (potentially different architectures of) machines needs to do that.  That process is cmirrord and the code that should translate for that daemon is in lvm2/daemons/cmirrord/compat.c.
>
> There could be a bug, but unless I'm not understanding right, we shouldn't need to switch endian in the kernel.
>
>   brassow
>
I think this is because cmirrord always assume the network package is 
little-endian.(see compat.c: v5_endian_from_network) .
So if cmirrord is running on S390, kernel will send package in its 
native endian(big endian), this is why data is broken.
There might have two solutions:

1. cmirrord always use native endian.
2. kernel always send little endian data.






--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] convert dm_ulog_request data to little endian

[Brassow Jonathan]

On Aug 19, 2014, at 10:19 PM, zhangdongmao wrote:

> 
> 于 2014年08月20日 08:58, Brassow Jonathan 写道:
>> This seems odd to me.  I don't understand why you would get a bug.  Do you have a mixed-architecture cluster?  Some x86-64 and the s390?  Otherwise, all operations should be in a format that is understood (unless there is a bug).
>> 
>> Also, you don't really need to translate the structure just to go to userspace.  Only the process that communicates between (potentially different architectures of) machines needs to do that.  That process is cmirrord and the code that should translate for that daemon is in lvm2/daemons/cmirrord/compat.c.
>> 
>> There could be a bug, but unless I'm not understanding right, we shouldn't need to switch endian in the kernel.
>> 
>>  brassow
>> 
> I think this is because cmirrord always assume the network package is little-endian.(see compat.c: v5_endian_from_network) .
> So if cmirrord is running on S390, kernel will send package in its native endian(big endian), this is why data is broken.
> There might have two solutions:
> 
> 1. cmirrord always use native endian.
> 2. kernel always send little endian data.

v5_endian_from_network() only gets called if it is needed.  I suspect there is a bug here, 'clog_request_to_network' should detect if big endian is being used and switch it to little endian.  In-coming communication must always be little endian.  I'm not sure which end the problem is on (I think the send side).  You could add some prints to detect the issue.

lvm2/daemons/cmirrord/cluster.c:cluster_send() sets two version numbers - one is forced to be little endian via xlate64().  'clog_request_to_network()' is then called and compares the two version numbers.  If they are different, the machine is big endian and the contents must be converted.  If this is not happening, it is bad.  Then, *from_network() performs a similar action and test and then must xlate back to big endian.  If this is not happening, it is bad.  So, for big endian machines, ensure that v5_endian_*_network() is being called.  (For little endian, it should not be.)

 brassow

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] convert dm_ulog_request data to little endian

[zhangdongmao]

> v5_endian_from_network() only gets called if it is needed.  I suspect there is a bug here, 'clog_request_to_network' should detect if big endian is being used and switch it to little endian.  In-coming communication must always be little endian.  I'm not sure which end the problem is on (I think the send side).  You could add some prints to detect the issue.
>
> lvm2/daemons/cmirrord/cluster.c:cluster_send() sets two version numbers - one is forced to be little endian via xlate64().  'clog_request_to_network()' is then called and compares the two version numbers.  If they are different, the machine is big endian and the contents must be converted.  If this is not happening, it is bad.  Then, *from_network() performs a similar action and test and then must xlate back to big endian.  If this is not happening, it is bad.  So, for big endian machines, ensure that v5_endian_*_network() is being called.  (For little endian, it should not be.)
>
>   brassow
hi,  brassow ,

       There might be a bug in v5_endian_to_network when sending packet.
       If the machine were big-endian,  v5_endian_to_network will 
convert request_type to little endian.
But in v5_data_endian_switch, it will check request_type again, and this 
check will failed because request_type is already converted
to little endian. So my solution to this is to delay xlate32 of 
u_rq->request_type.

      On the other hand, when receiving package in 
clog_request_from_network. I think the vp[0] will always be little endian.
we could use xlate64(vp[0]) == vp[0] to decide if the local node is 
little endian or not. if local node is little endian, we do nothing,
if the local node is big endian, we have to call v5_endian_from_network 
to convert little endian to big endian.

      But for now, I have not had a chance to test this patch on s390


here's the patch:

diff --git a/daemons/cmirrord/compat.c b/daemons/cmirrord/compat.c
index 3f7a9b5..c9cb803 100644
--- a/daemons/cmirrord/compat.c
+++ b/daemons/cmirrord/compat.c
@@ -126,13 +126,14 @@ static int v5_endian_to_network(struct 
clog_request *rq)

         u_rq->error = xlate32(u_rq->error);
         u_rq->seq = xlate32(u_rq->seq);
-       u_rq->request_type = xlate32(u_rq->request_type);
         u_rq->data_size = xlate64(u_rq->data_size);

         rq->originator = xlate32(rq->originator);

         v5_data_endian_switch(rq, 1);

+       rq->u_rq.request_type = xlate32(rq->u_rq.request_type);
+
         return size;
  }

@@ -187,7 +188,7 @@ int clog_request_from_network(void *data, size_t 
data_len)

         switch (version) {
         case 5: /* Upstream */
-               if (version == unconverted_version)
+               if (version == vp[0])
                         return 0;
                 break;
         case 4: /* RHEL 5.[45] */



Dongmao Zhang