blktap2: Invalid logic detecting unaligned buffers in vhd_write_block

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

---

tools/blktap2/vhd/lib/libvhd.c +1 -1

index 1fd5b4e..4ebe012 100644
@@ -2188,7 +2188,7 @@ vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data)
        if (block >= ctx->bat.entries)
                return -ERANGE;
 
-       if ((unsigned long)data & ~(VHD_SECTOR_SIZE -1))
+       if ((unsigned long)data & (VHD_SECTOR_SIZE -1))
                return -EINVAL;
 
        blk  = ctx->bat.bat[block];

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

Wei.

Quoted 19 lines

---

tools/blktap2/vhd/lib/libvhd.c +1 -1

index 1fd5b4e..4ebe012 100644
@@ -2188,7 +2188,7 @@ vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data)
        if (block >= ctx->bat.entries)
                return -ERANGE;
 
-       if ((unsigned long)data & ~(VHD_SECTOR_SIZE -1))
+       if ((unsigned long)data & (VHD_SECTOR_SIZE -1))
                return -EINVAL;
 
        blk  = ctx->bat.bat[block];

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 12 lines · Wei Liu

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Quoted 6 lines

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

We have an old fork of blktap2 from way back when. I was working to extract our changes and turn them into patches so we could rebase on upstream blktap2. Someone long ago found that - I have no idea how but it was a valid fix so I upstreamed it.

There are a number of other ones that were found that are still in upstream blktap2 - I plan to send them too.

Quoted 12 lines · Wei Liu

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Quoted 6 lines

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

We have an old fork of blktap2 from way back when. I was working to extract our changes and turn them into patches so we could rebase on upstream blktap2. Someone long ago found that - I have no idea how but it was a valid fix so I upstreamed it.

There are a number of other ones that were found that are still in upstream blktap2 - I plan to send them too.

Quoted 25 lines · Ross Philipson

Quoted 12 lines · Wei Liu

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Quoted 6 lines

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

We have an old fork of blktap2 from way back when. I was working to extract our changes and turn them into patches so we could rebase on upstream blktap2. Someone long ago found that - I have no idea how but it was a valid fix so I upstreamed it.

Quoted 2 lines

There are a number of other ones that were found that are still in upstream blktap2 - I plan to send them too.

Ross,

Instead of cross-porting fixes from XenServer's blktap3 to the bitrotting blktap2, would you consider taking up my work on replacing blktap2 with building blktap3 as an external project?

The basic shape of things that need to happen for that are:

1. Allow qemu to provide emulated devices for disks which use hotplug scripts (just checked in last week)

2. Do the tweaks necessary to allow blktap3 to be built as an independent project (outside the XenServer build system)

3. Switch libxl to use the already-in-tree block-tap script (which calls tapctl) rather than linking against the blktap library.

4. Add a blktap target to raisin.

#1 is done and was just checked in last week. #2 shouldn't be a terribly large amount of work. For #3, I've posted patches already, and I can probably do a quick rebase for you to pick up if you wanted. #4 shouldn't be too hard once you have an out-of-tree build working; I can help with that as well.

If we then add tests for upstream blktap to osstest, then we'll catch any breakages, and automatically get updates; and we can delete the bitrotting blktap2 out of the tree.

What do you think?

-George

Quoted 25 lines · Ross Philipson

Quoted 12 lines · Wei Liu

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Quoted 6 lines

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

We have an old fork of blktap2 from way back when. I was working to extract our changes and turn them into patches so we could rebase on upstream blktap2. Someone long ago found that - I have no idea how but it was a valid fix so I upstreamed it.

Quoted 2 lines

There are a number of other ones that were found that are still in upstream blktap2 - I plan to send them too.

Ross,

Instead of cross-porting fixes from XenServer's blktap3 to the bitrotting blktap2, would you consider taking up my work on replacing blktap2 with building blktap3 as an external project?

The basic shape of things that need to happen for that are:

1. Allow qemu to provide emulated devices for disks which use hotplug scripts (just checked in last week)

2. Do the tweaks necessary to allow blktap3 to be built as an independent project (outside the XenServer build system)

3. Switch libxl to use the already-in-tree block-tap script (which calls tapctl) rather than linking against the blktap library.

4. Add a blktap target to raisin.

#1 is done and was just checked in last week. #2 shouldn't be a terribly large amount of work. For #3, I've posted patches already, and I can probably do a quick rebase for you to pick up if you wanted. #4 shouldn't be too hard once you have an out-of-tree build working; I can help with that as well.

If we then add tests for upstream blktap to osstest, then we'll catch any breakages, and automatically get updates; and we can delete the bitrotting blktap2 out of the tree.

What do you think?

-George

Quoted 27 lines · George Dunlap

Quoted 25 lines · Ross Philipson

Quoted 12 lines · Wei Liu

Quoted 7 lines · Ross Philipson

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so:

Quoted 6 lines

Acked-by Wei Liu <[email protected]>

But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change.

OOI if no code was using this, how did you discover this problem?

We have an old fork of blktap2 from way back when. I was working to extract our changes and turn them into patches so we could rebase on upstream blktap2. Someone long ago found that - I have no idea how but it was a valid fix so I upstreamed it.

Quoted 8 lines

Quoted 2 lines

There are a number of other ones that were found that are still in upstream blktap2 - I plan to send them too.

Ross,

Instead of cross-porting fixes from XenServer's blktap3 to the bitrotting blktap2, would you consider taking up my work on replacing blktap2 with building blktap3 as an external project?

George, a few questions below to get started...

Quoted 8 lines

The basic shape of things that need to happen for that are:

1. Allow qemu to provide emulated devices for disks which use hotplug scripts (just checked in last week)

2. Do the tweaks necessary to allow blktap3 to be built as an independent project (outside the XenServer build system)

I am not sure where the latest and greatest blktap3 work can be found. I have found some old repos that have activity up until the work stopped on that project in 2013. Can you give me some pointers on this?

Also it seems the overall idea is to not have blktap3 (or any blktap) live in the Xen tree but rather be external. Where would blktap3 live?

Quoted 3 lines

3. Switch libxl to use the already-in-tree block-tap script (which calls tapctl) rather than linking against the blktap library.

I believe I found this work:

http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg00805.html

It looks like 4 of the set made it in and patches 3 & 5 still need work? I can re-base them.

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

---

tools/blktap2/vhd/lib/libvhd.c +0 -0

index 1fd5b4e..4ebe012 100644

Ross Philipson writes ("[Xen-devel] [PATCH] blktap2: Invalid logic detecting unaligned buffers in vhd_write_block"):

Quoted 13 lines

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

---

tools/blktap2/vhd/lib/libvhd.c +0 -0

index 1fd5b4e..4ebe012 100644

tools/blktap2/vhd/lib/libvhd.c vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data) · line 2191

Quoted 5 lines

tools/blktap2/vhd/lib/libvhd.c +1 -1

@@ -2188,7 +2188,7 @@ vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data)
        if (block >= ctx->bat.entries)
                return -ERANGE;
 
-       if ((unsigned long)data & ~(VHD_SECTOR_SIZE -1))
+       if ((unsigned long)data & (VHD_SECTOR_SIZE -1))

Notwithstanding the discussion in the rest of the thread, this is a clearly-correct bugfix to the code in tree, so I have queued it.

However, I had to do so rather manually because something had mangled the whitespace. Ross, can you check your patch submission path, please ?

Ian.

Ross Philipson writes ("[Xen-devel] [PATCH] blktap2: Invalid logic detecting unaligned buffers in vhd_write_block"):

Quoted 13 lines

It seems the logic is meant to detect sector unaligned buffers for block writes. The NOTing of the logic instead masks off any unaligned bits and also would cause the function to always fail. It seems the function is not used in any of the tools so that is probably why the problem is not seen. In the vhd_read_block function it is correct.

Signed-off-by Ross Philipson <[email protected]>

---

tools/blktap2/vhd/lib/libvhd.c +0 -0

index 1fd5b4e..4ebe012 100644

tools/blktap2/vhd/lib/libvhd.c vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data) · line 2191

Quoted 5 lines

tools/blktap2/vhd/lib/libvhd.c +1 -1

@@ -2188,7 +2188,7 @@ vhd_write_block(vhd_context_t *ctx, uint32_t block, char *data)
        if (block >= ctx->bat.entries)
                return -ERANGE;
 
-       if ((unsigned long)data & ~(VHD_SECTOR_SIZE -1))
+       if ((unsigned long)data & (VHD_SECTOR_SIZE -1))

Notwithstanding the discussion in the rest of the thread, this is a clearly-correct bugfix to the code in tree, so I have queued it.

However, I had to do so rather manually because something had mangled the whitespace. Ross, can you check your patch submission path, please ?

Ian.

Quoted 3 lines

However, I had to do so rather manually because something had mangled the whitespace. Ross, can you check your patch submission path, please ?

Never mind, I found the problem. Tabs got turned into spaces. Sorry about that.