Bug #1863
closed
Implement 'hammer volume-list' subcommand
Added by Anonymous over 13 years ago. Updated over 13 years ago.
0%
Description
First attempt to add a 'hammer volume-list' subcommand, that takes as argument a
filesystem and returns a list of volumes (device nodes) that make up that
filesystem.
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
Cheers,
Stathis
P.S. If anyone is willing to instead make mount(8) output the volumes for a
filesystem, it's fine by me.
Updated by Anonymous over 13 years ago
Short follow-up.
Matt commented on the code in IRC and said that there should a validation of
sizeof(struct hammer_ioc_volume). Otherwise the hammer vfs might overflow the
data buffer, the userland provides.
Although Matt was kind enough to explain it twice, I still don't get it. I'm
allocating room for the maximum volumes a file system can have and also I'm only
writing to the 'device_name' field of 'hammer_ioc_volume' structure, which
happens to have automatic storage.
So, what kind of buffer overrun I should be checking against? Can anyone please
provide some code snippet or an insight ?
Thanks!
Stathis
Updated by aoiko over 13 years ago
On 10/07/2010 08:02 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
Stathis Kamperis <ekamperi@gmail.com> added the comment:
Short follow-up.
Matt commented on the code in IRC and said that there should a validation of
sizeof(struct hammer_ioc_volume). Otherwise the hammer vfs might overflow the
data buffer, the userland provides.Although Matt was kind enough to explain it twice, I still don't get it. I'm
allocating room for the maximum volumes a file system can have and also I'm only
writing to the 'device_name' field of 'hammer_ioc_volume' structure, which
happens to have automatic storage.
I haven't read the irc discussion, but skimming through your patch I see
the following issues:
- In hammer_ioc_volume_list() you seem to be directly dereferencing the
userland pointer. You should be using copyin() and friends to bring in
the actual pointer and the count.
- I can see you are correctly specifying the max volume number in
hammer(8), but the issue is that the kernel side should be making sure
not to overrun the buffer space. Your kernel loop copies
HAMMER_MAX_VOLUMES irrespective of how much space (possibly zero) the
user side has specified. Your hammer(8) happens to behave correctly, but
anyone can compile and run a program that will panic the kernel.
- Again, you cannot just strlcpy() stuff to userspace. The page might
not be present or the address could be completely invalid. Use copyout
instead.
- I see no need to define an extra structure. Just pass the two fields
in as arguments to the ioctl().
Nice job otherwise.
HTH,
Aggelos
Updated by aoiko over 13 years ago
On 10/07/2010 10:17 PM, Aggelos Economopoulos wrote:
On 10/07/2010 08:02 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:Stathis Kamperis <ekamperi@gmail.com> added the comment:
Short follow-up.
Matt commented on the code in IRC and said that there should a validation of
sizeof(struct hammer_ioc_volume). Otherwise the hammer vfs might overflow the
data buffer, the userland provides.Although Matt was kind enough to explain it twice, I still don't get it. I'm
allocating room for the maximum volumes a file system can have and also I'm only
writing to the 'device_name' field of 'hammer_ioc_volume' structure, which
happens to have automatic storage.I haven't read the irc discussion, but skimming through your patch I see
the following issues:
- In hammer_ioc_volume_list() you seem to be directly dereferencing the
userland pointer. You should be using copyin() and friends to bring in
the actual pointer and the count.
Ah, ioctl automagically copies in the struct. So no problem there.
- I can see you are correctly specifying the max volume number in
hammer(8), but the issue is that the kernel side should be making sure
not to overrun the buffer space. Your kernel loop copies
HAMMER_MAX_VOLUMES irrespective of how much space (possibly zero) the
user side has specified. Your hammer(8) happens to behave correctly, but
anyone can compile and run a program that will panic the kernel.
- Again, you cannot just strlcpy() stuff to userspace. The page might
not be present or the address could be completely invalid. Use copyout
instead.
- I see no need to define an extra structure. Just pass the two fields
in as arguments to the ioctl().
Silly me, of course you need to use the structure. The other two issues
are valid though.
Aggelos
Updated by Anonymous over 13 years ago
Thanks Aggelos. I'll fix the issues you raised.
Cheers,
Stathis
Updated by Anonymous over 13 years ago
I updated the patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
Regards,
Stathis
Updated by josepht over 13 years ago
On Fri, Oct 08, 2010 at 04:01:22PM +0000, Stathis Kamperis (via DragonFly issue tracker) wrote:
Stathis Kamperis <ekamperi@gmail.com> added the comment:
I updated the patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
Link seems broken.
Joe
Updated by Anonymous over 13 years ago
Fixed, sorry.
I changed strlcpy() to copyout() and also added some rudimentary validation on
the count of volumes to write.
I honestly don't know how to make kernel defend against a lying user-land.
Regards,
Stathis
Updated by aoiko over 13 years ago
On 10/08/2010 06:26 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
Stathis Kamperis <ekamperi@gmail.com> added the comment:
Fixed, sorry.
I changed strlcpy() to copyout() and also added some rudimentary validation on
the count of volumes to write.I honestly don't know how to make kernel defend against a lying user-land.
The kernel holds all the cards, just check absolutely everything
userland tells you against your own data structures :)
Patch looks ok, except that you don't want to unconditionally copy
MAXPATHLEN bytes to userland. The issue here isn't performance, but
information disclosure. Your code will copy to userspace whatever
happens to be allocated adjacent to the volume name in RAM. Perhaps an
implementation of copyoutstr() is in order?
HTH,
Aggelos
Updated by dillon over 13 years ago
ioctls copy data into/out of kernel space for you, but the
size of the structure is built into the ioctl command so it
cannot change without changing the ioctl command. It's also
fairly limited. So anything that is potentially large needs
to be passed in as a separate user pointer embedded in
the ioctl command structure.
-Matt
Matthew Dillon
<dillon@backplane.com>
Updated by Anonymous over 13 years ago
New patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
Stathis
Updated by aoiko over 13 years ago
On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
Stathis Kamperis <ekamperi@gmail.com> added the comment:
New patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
Can you explain why you use the 0001- prefix if you're going to use the
same url over and over again? It's just extra pain for us to rename your
patches, which is something I did, so now I can do the following, in
order to only review your changes relative to the last patch. Comments
inline.
$ diff u 000{1,2}-HAMMER-Implement-volume-list-command.patch+ int i, cnt;
+ int i, cnt, len;
+
+ for (i = 0, cnt = 0; i < HAMMER_MAX_VOLUMES && cnt < ioc->nvols;
i+) {
+ volume = hammer_get_volume(hmp, i, &error);@ -213,8 +213,13 @
+ }
+ KKASSERT;
+ len = strlen(volume->vol_name) + 1;
+ if (len > MAXPATHLEN) {
+ len = MAXPATHLEN;
+ volume->vol_name[len - 1] = '\0';
+ }
+ error = copyout(volume->vol_name,
ioc->vols[cnt].device_name,
-+ MAXPATHLEN);
++ len);
Looks good, although you should probably just KKASSERT(len <
MAXPATHLEN). Please commit.
Aggelos
Updated by swildner over 13 years ago
On 10/19/2010 23:22, Aggelos Economopoulos wrote:
On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:Stathis Kamperis<ekamperi@gmail.com> added the comment:
New patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patchCan you explain why you use the 0001- prefix if you're going to use the
same url over and over again? It's just extra pain for us to rename your
patches, which is something I did, so now I can do the following, in
order to only review your changes relative to the last patch.
[...]
fetch -o ...
Sascha
Updated by aoiko over 13 years ago
On 10/20/2010 02:00 AM, Sascha Wildner wrote:
On 10/19/2010 23:22, Aggelos Economopoulos wrote:
On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:Stathis Kamperis<ekamperi@gmail.com> added the comment:
New patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patchCan you explain why you use the 0001- prefix if you're going to use the
same url over and over again? It's just extra pain for us to rename your
patches, which is something I did, so now I can do the following, in
order to only review your changes relative to the last patch.
[...]fetch -o ...
Yes, that is the renaming I was refering to. The fact that it is easy
to do does not address the problems I pointed out, namely that it is
pointless to have a serial counter prefix and not increment it.
Addditionally, rewriting the patch means that it is the reviewers who
need to bother with keeping history (which, as demonstrated, is
useful) and of course I can no longer refer unambiguously to version 1
of the patch (My local numbering after the renames is 0-2. What's yours?).
Aggelos
Updated by Anonymous over 13 years ago
git format-patch 1 automatically added the 0001 prefix, since I re-based my
patch and made it appear on top of the commit history.
Sorry for the inconvenience.
Speaking of fetch(1), could we make it automatically append an increasing
counter if the file to be fetched already exists on disk ?
Stathis
Updated by dillon over 13 years ago
:git format-patch 1 automatically added the 0001 prefix, since I re-based my
:patch and made it appear on top of the commit history.
:
:Sorry for the inconvenience.
:
:Speaking of fetch(1), could we make it automatically append an increasing
:counter if the file to be fetched already exists on disk ?
:
:Stathis
I'd rather not. Many programs rely on fetch to operate just like it
does now.-Matt
Matthew Dillon
<dillon@backplane.com>