Patchwork e2fslibs: fix llseek on i386

login
register
mail settings
Submitter Phillip Susi
Date Jan. 24, 2013, 4:21 p.m.
Message ID <1359044517-18243-1-git-send-email-psusi@ubuntu.com>
Download mbox | patch
Permalink /patch/215426/
State Superseded
Headers show

Comments

Phillip Susi - Jan. 24, 2013, 4:21 p.m.
ext2fs_llseek() was using lseek instead of lseek64.  The
only time it would use lseek64 is if passed an offset that
overflowed 32 bits.  This works for SEEK_SET, but not
SEEK_CUR, which can apply a small offset to move the file
pointer past the 32 bit limit.

The code has been changed to instead try lseek64 first, and
fall back to lseek if that fails.  It also was doing a
runtime check of the size of off_t.  This has been moved to
compile time.

Signed-off-by: Phillip Susi <psusi@ubuntu.com>
---
 configure.in        |    3 +++
 lib/config.h.in     |    3 +++
 lib/ext2fs/llseek.c |   13 +++++--------
 3 files changed, 11 insertions(+), 8 deletions(-)
Theodore Ts'o - Jan. 24, 2013, 7:51 p.m.
On Thu, Jan 24, 2013 at 11:21:56AM -0500, Phillip Susi wrote:
> ext2fs_llseek() was using lseek instead of lseek64.  The
> only time it would use lseek64 is if passed an offset that
> overflowed 32 bits.  This works for SEEK_SET, but not
> SEEK_CUR, which can apply a small offset to move the file
> pointer past the 32 bit limit.
> 
> The code has been changed to instead try lseek64 first, and
> fall back to lseek if that fails.  It also was doing a
> runtime check of the size of off_t.  This has been moved to
> compile time.
> 
> Signed-off-by: Phillip Susi <psusi@ubuntu.com>

How did you find this?  I've done a quick search for SEEK_CUR, and it
looks like only place where this could cause a problem is with
e2image.  And a quick test of a i386 version of e2image with a large
file system is that it does indeed blow up with an "Inappropriate
ioctl for device" error.

Is there any other potential problems that are caused by this bug?  I
like to explain the impacts of bug fixes in libext2fs for folks who
are doing bug fix / code archeology.

Thanks,


					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi - Jan. 24, 2013, 8:22 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/24/2013 2:51 PM, Theodore Ts'o wrote:
> How did you find this?  I've done a quick search for SEEK_CUR, and
> it looks like only place where this could cause a problem is with 
> e2image.  And a quick test of a i386 version of e2image with a
> large file system is that it does indeed blow up with an
> "Inappropriate ioctl for device" error.

That's where I found it, but the error should be "seek: Value too
large for defined data type"

> Is there any other potential problems that are caused by this bug?
> I like to explain the impacts of bug fixes in libext2fs for folks
> who are doing bug fix / code archeology.

If e2image is the only internal user of the call with SEEK_CUR, then I
guess it only affects any external users of the library who were doing
this ( I am not aware of any ).


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJRAZgNAAoJEJrBOlT6nu752g0IAN4czK0LevEa9Mx2htaZ3oms
TtcpKe7tuXgmvLB0XWv8Q2f6zCDMH7qmWqWZORdx98jg2XeynaEbZvVIQFEsfOVa
05r7sk64TPGXrdkwuqFTe4M7MYmrQmLnfGzd2NMddlWcXOc+R4pFUrrvYbU7QQv/
SGfb9Uo+EFZhSuJLPY1QW4EO09ghD4k6XHnHtioAsgjyxSa4p2NcZiBxmFT7ZJdw
DbET65wHkKT2s+jLQQAOPx/IYAftrZuV7t+YoX7RTS4tdpENlgoVlZ3drCvHB2aG
LwDDns4C1e0cAMncGKh3HY4/CInMZ1jFWHYW+VvuzZNJ7Uxur2mG6NUyWp6cNqU=
=WeEv
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Jan. 24, 2013, 8:32 p.m.
On Thu, Jan 24, 2013 at 03:22:37PM -0500, Phillip Susi wrote:
> 
> On 1/24/2013 2:51 PM, Theodore Ts'o wrote:
> > How did you find this?  I've done a quick search for SEEK_CUR, and
> > it looks like only place where this could cause a problem is with 
> > e2image.  And a quick test of a i386 version of e2image with a
> > large file system is that it does indeed blow up with an
> > "Inappropriate ioctl for device" error.
> 
> That's where I found it, but the error should be "seek: Value too
> large for defined data type"

Well, I did my testing using an i386 debian/testing chroot running
under a x86-64 3.8.0-rc3 kernel.  I'm guessing it was the use of a
32-bit userspace / 64-bit kernel that probably explains the
difference.

> > Is there any other potential problems that are caused by this bug?
> > I like to explain the impacts of bug fixes in libext2fs for folks
> > who are doing bug fix / code archeology.
> 
> If e2image is the only internal user of the call with SEEK_CUR, then I
> guess it only affects any external users of the library who were doing
> this ( I am not aware of any ).

Well, there are some binaries that aren't usually built by most
distributions (make-sparse and copy-sparse), but in terms of primary
e2fsprogs programs (mke2fs, e2fskc, tune2fs, chattr, lsattr, etc.)
nope, none of them use SEEK_CUR.

The lib/ext2fs/fileio.c file does use SEEK_CUR, which means it might
impact 3rd party packages such as e2tools and ext2fuse (although
that's generally only used on Mac and Windows systems).

Cheers,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Jan. 25, 2013, 2:16 a.m.
On Fri, Jan 25, 2013 at 10:25:14AM +0800, Zheng Liu wrote:
> 
> This patch makes me consider my patches that dump a sparse file in
> debugfs.  In my patch [1] llseek64(2) is called to seek to the next
> data in target file.  So I believe ext2fs_llseek() is a better choice.
> I am happy to send a newer patch to fix it.  What do you think?
> 
> 1. http://www.spinics.net/lists/linux-ext4/msg36134.html

You're right, we should use ext2fs_llseek() for better portability.
Thanks,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu - Jan. 25, 2013, 2:25 a.m.
On Thu, Jan 24, 2013 at 03:32:30PM -0500, Theodore Ts'o wrote:
> On Thu, Jan 24, 2013 at 03:22:37PM -0500, Phillip Susi wrote:
> > 
> > On 1/24/2013 2:51 PM, Theodore Ts'o wrote:
> > > How did you find this?  I've done a quick search for SEEK_CUR, and
> > > it looks like only place where this could cause a problem is with 
> > > e2image.  And a quick test of a i386 version of e2image with a
> > > large file system is that it does indeed blow up with an
> > > "Inappropriate ioctl for device" error.
> > 
> > That's where I found it, but the error should be "seek: Value too
> > large for defined data type"
> 
> Well, I did my testing using an i386 debian/testing chroot running
> under a x86-64 3.8.0-rc3 kernel.  I'm guessing it was the use of a
> 32-bit userspace / 64-bit kernel that probably explains the
> difference.
> 
> > > Is there any other potential problems that are caused by this bug?
> > > I like to explain the impacts of bug fixes in libext2fs for folks
> > > who are doing bug fix / code archeology.
> > 
> > If e2image is the only internal user of the call with SEEK_CUR, then I
> > guess it only affects any external users of the library who were doing
> > this ( I am not aware of any ).
> 
> Well, there are some binaries that aren't usually built by most
> distributions (make-sparse and copy-sparse), but in terms of primary
> e2fsprogs programs (mke2fs, e2fskc, tune2fs, chattr, lsattr, etc.)
> nope, none of them use SEEK_CUR.
> 
> The lib/ext2fs/fileio.c file does use SEEK_CUR, which means it might
> impact 3rd party packages such as e2tools and ext2fuse (although
> that's generally only used on Mac and Windows systems).

Hi Ted,

This patch makes me consider my patches that dump a sparse file in
debugfs.  In my patch [1] llseek64(2) is called to seek to the next
data in target file.  So I believe ext2fs_llseek() is a better choice.
I am happy to send a newer patch to fix it.  What do you think?

1. http://www.spinics.net/lists/linux-ext4/msg36134.html

Thanks,
                                                - Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu - Jan. 25, 2013, 2:48 a.m.
On Thu, Jan 24, 2013 at 09:16:05PM -0500, Theodore Ts'o wrote:
> On Fri, Jan 25, 2013 at 10:25:14AM +0800, Zheng Liu wrote:
> > 
> > This patch makes me consider my patches that dump a sparse file in
> > debugfs.  In my patch [1] llseek64(2) is called to seek to the next
> > data in target file.  So I believe ext2fs_llseek() is a better choice.
> > I am happy to send a newer patch to fix it.  What do you think?
> > 
> > 1. http://www.spinics.net/lists/linux-ext4/msg36134.html
> 
> You're right, we should use ext2fs_llseek() for better portability.
> Thanks,

Yep, the next version will be sent out soon.

Regards,
                                                - Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Jan. 25, 2013, 4:13 a.m.
On Thu, Jan 24, 2013 at 11:21:56AM -0500, Phillip Susi wrote:
> ext2fs_llseek() was using lseek instead of lseek64.  The
> only time it would use lseek64 is if passed an offset that
> overflowed 32 bits.  This works for SEEK_SET, but not
> SEEK_CUR, which can apply a small offset to move the file
> pointer past the 32 bit limit.
> 
> The code has been changed to instead try lseek64 first, and
> fall back to lseek if that fails.  It also was doing a
> runtime check of the size of off_t.  This has been moved to
> compile time.
> 
> Signed-off-by: Phillip Susi <psusi@ubuntu.com>

Applied, although I had to make a small change so that in the case
where llseek doesn't exist, we need to retry with lseek immediately.
Also, in the fallback case, we still need to make the runtime check to
assure that the requested offset is less than off_t.

On the 1.43 branch it probably makes sense to get rid of the whole
mess with my_llseek and just use llseek64 directly.  There's a bunch
of complexity here which because over a decade ago, glibc had a buggy
header files which caused llseek64 to malfunction (which is why I
didn't enable use of llseek64 and > 2GB file system support in the
first place), and the Debian developer at the time tried to be too
clever and shipped a patched version of e2fsprogs which tried to use
llseek64 without consulting me first, and then users started losing
data and having the file systems corrupted as a result.

So as a result, because I couldn't trust glibc to give me a non-buggy
version of llseek64, and Debian users were losing data in real time, I
had e2fsprogs call the system call directly.  It's been a long time
since systems with this particular glibc Fail have been around, so
eventually I should rip out a lot of this mess.

       	   				- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/configure.in b/configure.in
index ac3cd92..181faeb 100644
--- a/configure.in
+++ b/configure.in
@@ -970,14 +970,17 @@  AC_CHECK_SIZEOF(short)
 AC_CHECK_SIZEOF(int)
 AC_CHECK_SIZEOF(long)
 AC_CHECK_SIZEOF(long long)
+AC_CHECK_SIZEOF(off_t)
 SIZEOF_SHORT=$ac_cv_sizeof_short
 SIZEOF_INT=$ac_cv_sizeof_int
 SIZEOF_LONG=$ac_cv_sizeof_long
 SIZEOF_LONG_LONG=$ac_cv_sizeof_long_long
+SIZEOF_OFF_T=$ac_cv_sizeof_off_t
 AC_SUBST(SIZEOF_SHORT)
 AC_SUBST(SIZEOF_INT)
 AC_SUBST(SIZEOF_LONG)
 AC_SUBST(SIZEOF_LONG_LONG)
+AC_SUBST(SIZEOF_OFF_T)
 AC_C_BIGENDIAN
 BUILD_CC="$BUILD_CC" CPP="$CPP" /bin/sh $ac_aux_dir/parse-types.sh
 ASM_TYPES_HEADER=./asm_types.h
diff --git a/lib/config.h.in b/lib/config.h.in
index 6f88c9c..eebb75d 100644
--- a/lib/config.h.in
+++ b/lib/config.h.in
@@ -552,6 +552,9 @@ 
 /* The size of `long long', as computed by sizeof. */
 #undef SIZEOF_LONG_LONG
 
+/* The size of `off_t', as computed by sizeof. */
+#undef SIZEOF_OFF_T
+
 /* The size of `short', as computed by sizeof. */
 #undef SIZEOF_SHORT
 
diff --git a/lib/ext2fs/llseek.c b/lib/ext2fs/llseek.c
index b0576e4..e5cb784 100644
--- a/lib/ext2fs/llseek.c
+++ b/lib/ext2fs/llseek.c
@@ -90,18 +90,14 @@  static ext2_loff_t my_llseek (int fd, ext2_loff_t offset, int origin)
 
 ext2_loff_t ext2fs_llseek (int fd, ext2_loff_t offset, int origin)
 {
+#if SIZEOF_OFF_T >= SIZEOF_LONG_LONG
+	return lseek (fd, offset, origin);
+#else
 	ext2_loff_t result;
 	static int do_compat = 0;
 
-	if ((sizeof(off_t) >= sizeof(ext2_loff_t)) ||
-	    (offset < ((ext2_loff_t) 1 << ((sizeof(off_t)*8) -1))))
+	if (do_compat)
 		return lseek(fd, (off_t) offset, origin);
-
-	if (do_compat) {
-		errno = EINVAL;
-		return -1;
-	}
-
 	result = my_llseek (fd, offset, origin);
 	if (result == -1 && errno == ENOSYS) {
 		/*
@@ -112,6 +108,7 @@  ext2_loff_t ext2fs_llseek (int fd, ext2_loff_t offset, int origin)
 		errno = EINVAL;
 	}
 	return result;
+#endif
 }
 
 #else /* !linux */