headers
Kevin Jamieson | 1 Feb 2009 01:12

dm_get_dirattrs can write past end of user buffer

I have observed segfaults with an application that calls dm_get_dirattrs
with the latest DMAPI kernel built from SGI CVS.

The problem appears to be in dm_filldir:

	needed = dm_stat_size(namelen + 1);
	...
	error = -xfs_dm_bulkattr_iget_one(cb->mp, ino, 0,
		statp, needed);
	...
	/*
	 * On return from bulkstat_one(), stap->_link points
	 * at the end of the handle in the stat structure.
	 */
	statp->dt_compname.vd_offset = statp->_link;
	statp->dt_compname.vd_length = namelen + 1;

xfs_dm_bulkattr_iget_one() sets statp->_link to needed, so the name ends
up getting written past the space reserved for the name, which can
exceed cb->spaceleft.

The below patch appears to fix the problem.

Index: fs/xfs/dmapi/xfs_dm.c
===================================================================
RCS file: /cvs/linux-2.6-xfs/fs/xfs/dmapi/xfs_dm.c,v
retrieving revision 1.81
diff -u -r1.81 xfs_dm.c
--- fs/xfs/dmapi/xfs_dm.c	28 Oct 2008 05:39:09 -0000	1.81
+++ fs/xfs/dmapi/xfs_dm.c	31 Jan 2009 23:08:07 -0000
(Continue reading)

Permalink | Reply | 1 comment |
headers
Dave Chinner | 1 Feb 2009 01:37

Re: XFS Kernel 2.6.27.7 oopses

On Fri, Jan 30, 2009 at 11:23:59PM +0100, Ralf Liebenow wrote:
> Hello !
> 
> I heavily use XFS for an incremental backup server (by using rsync --link-dest option
> to create hardlinks to unchanged files), and therefore have about 10 million files
> on my TB Harddisk. To remove old versions nightly an "rm -rf" will remove a million
> hardlinks/files every night.
> 
> After a while I had regular oopses and so I updated the system to make sure its
> on a current version.
> 
> It is now a SuSE 11.1 64Bit with SuSE's Kernel 2.6.27.7-9-default

What kernel did you originally see this problem on?

> <4>Call Trace:
> <4> [<ffffffff8023219a>] complete+0x38/0x4b
> <4> [<ffffffffa00f5316>] xfs_iflush+0x73/0x2ab [xfs]
> <4> [<ffffffffa010a7a2>] xfs_finish_reclaim+0x12a/0x168 [xfs]
> <4> [<ffffffffa010a871>] xfs_finish_reclaim_all+0x91/0xcb [xfs]
> <4> [<ffffffffa010925c>] xfs_syncsub+0x50/0x22b [xfs]
> <4> [<ffffffffa0118a3a>] xfs_sync_worker+0x17/0x36 [xfs]
> <4> [<ffffffffa01189d4>] xfssyncd+0x15d/0x1ac [xfs]
> <4> [<ffffffff8025434d>] kthread+0x47/0x73
> <4> [<ffffffff8020d7b9>] child_rip+0xa/0x11

That may be a use after free. I know lachlan fixed a few in this
area, but I'm not sure what release those fixeѕ ended up in....

> What do you recommend ? Has this bug already been addressed within the
(Continue reading)

Permalink | Reply | 5 comments |
headers
Dave Chinner | 1 Feb 2009 01:54

Re: [PATCH 11/17] xfs: merge xfs_inode_flush into xfs_fs_write_inode

On Mon, Jan 26, 2009 at 02:31:47AM -0500, Christoph Hellwig wrote:
> Spliting the task for a VFS-induced inode flush into two functions doesn't

Splitting

> make any sense, so merge the two functions dealing with it.
.....
> +		error = xfs_iflush(ip, XFS_IFLUSH_ASYNC_NOBLOCK);
>  	}
> -	error = xfs_inode_flush(ip, flags);
>  
> -out_error:
> + out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> + out:

Don't need spaces before the labels.

Otherwise looks OK.

Cheers,

Dave.
--

-- 
Dave Chinner
david <at> fromorbit.com

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
(Continue reading)

Permalink | Reply | 0 comments |
headers
Christoph Hellwig | 1 Feb 2009 06:07
Favicon

Re: [PATCH] Apply gettext translation to more strings.

Looks good to me.  I'll put this in if you promise to send an updated
polish translation ASAP so we can cut 3.0.0 :)

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply | 0 comments |
headers
Christoph Hellwig | 1 Feb 2009 06:09
Favicon

Re: dm_get_dirattrs can write past end of user buffer

On Sat, Jan 31, 2009 at 04:12:52PM -0800, Kevin Jamieson wrote:
> The below patch appears to fix the problem.

The patch looks okay, but I don't think there's any uptodate tree
that includes dmapi support.

Btw, we'd also need to have a

	Signed-off-by: Kevin Jamieson <kevin <at> kevinjamieson.com>

please read Documentation/SubmittingPatches in the kernel tree and make
sure you agree with the conditions for the Signed-off-by:.

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply | 0 comments |
headers
Christoph Hellwig | 1 Feb 2009 09:12
Favicon

reproducible xfs/vmap oops

When running xfsqa on the current xfs development tree (sometimes two
runs are needed to trigger it) I get the following oops.  This seems to
have been introduced by the last mainling merge (with
5ee810072175042775e39bdd3eaaa68884c27805), but I'd need to bisect it to
make sure my gut feeling is right.

[ 3262.460241] XFS mounting filesystem vde
[ 3262.474253] ------------[ cut here ]------------
[ 3262.476024] kernel BUG at mm/vmalloc.c:294!
[ 3262.476024] invalid opcode: 0000 [#1] SMP 
[ 3262.476024] last sysfs file: /sys/class/net/lo/operstate
[ 3262.476024] Modules linked in:
[ 3262.476024] 
[ 3262.476024] Pid: 31907, comm: mount Not tainted (2.6.29-rc2-xfs #30) 
[ 3262.476024] EIP: 0060:[<c01b4b28>] EFLAGS: 00010207 CPU: 0
[ 3262.476024] EIP is at __insert_vmap_area+0x88/0xb0
[ 3262.476024] EAX: 00101000 EBX: ffd01000 ECX: 00000000 EDX: f5f25df4
[ 3262.476024] ESI: d2180340 EDI: d2180340 EBP: f32f5bcc ESP: f32f5bc4
[ 3262.476024]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[ 3262.476024] Process mount (pid: 31907, ti=f32f4000 task=de55e2b0
task.ti=f32f4000)
[ 3262.476024] Stack:
[ 3262.476024]  ffd01000 f874e000 f32f5c10 c01b5581 00000000 000200d0
00001000 f8b4e000
[ 3262.476024]  00400000 d2180340 00000000 d2180340 00000fff fffff000
c0a69e80 c0a69e80
[ 3262.476024]  f685ca98 00000400 00000400 f32f5c58 c01b5972 fff4e000
ffffffff 000000d0
[ 3262.476024] Call Trace:
[ 3262.476024]  [<c01b5581>] ? alloc_vmap_area+0x1b1/0x220
(Continue reading)

Permalink | Reply | 3 comments |
headers
Boaz Harrosh | 1 Feb 2009 10:48
Favicon
Gravatar

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

Arnd Bergmann wrote:
> On Saturday 31 January 2009, Andrew Morton wrote:
>> Is this written in a standard somewhere?  Is it guaranteed?
> 
> Alignment is defined in the architecture psABI documents. 
> Unfortunately, many of them were written before the 'long long'
> type became part of the C standard, so it's not strictly guaranteed.
> AFAICT, the alignment of __u64 on x86 is the same as the alignment
> of 'double' by convention.
> 
> However, the problem is well-understood: x86 is the only one
> that has a problem in 32/64 bit compat mode. m68k has similar
> issues with 16/32 bit integers, but those don't apply here.
> 
>> If some (perhaps non-gcc) compiler were to lay this out differently
>> (perhaps with suitable command-line options) then that's liveable
>> with - as long as the kernel never changes the layout.  Of course
>> it would be better to avoid this if poss.
> 
> If a compiler was using irregular structure alignment, all sorts of
> library interfaces would break. The kernel ABI is only a small part
> of the problem then.
> 
>> The other potential issue with a structure like this is that there's a
>> risk that it will lead us to copy four bytes of uninitialised kernel
>> memory out to userspace.
>>
>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>> for this sort of thing.
>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Geert Uytterhoeven | 1 Feb 2009 11:05

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Saturday 31 January 2009, Andrew Morton wrote:
> >> Is this written in a standard somewhere?  Is it guaranteed?
> > 
> > Alignment is defined in the architecture psABI documents. 
> > Unfortunately, many of them were written before the 'long long'
> > type became part of the C standard, so it's not strictly guaranteed.
> > AFAICT, the alignment of __u64 on x86 is the same as the alignment
> > of 'double' by convention.
> > 
> > However, the problem is well-understood: x86 is the only one
> > that has a problem in 32/64 bit compat mode. m68k has similar
> > issues with 16/32 bit integers, but those don't apply here.
> > 
> >> If some (perhaps non-gcc) compiler were to lay this out differently
> >> (perhaps with suitable command-line options) then that's liveable
> >> with - as long as the kernel never changes the layout.  Of course
> >> it would be better to avoid this if poss.
> > 
> > If a compiler was using irregular structure alignment, all sorts of
> > library interfaces would break. The kernel ABI is only a small part
> > of the problem then.
> > 
> >> The other potential issue with a structure like this is that there's a
> >> risk that it will lead us to copy four bytes of uninitialised kernel
> >> memory out to userspace.
> >>
> >> IOW, it seems a generally bad idea to rely upon compiler-added padding
> >> for this sort of thing.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Boaz Harrosh | 1 Feb 2009 11:39
Favicon
Gravatar

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> On Saturday 31 January 2009, Andrew Morton wrote:
>>>> Is this written in a standard somewhere?  Is it guaranteed?
>>> Alignment is defined in the architecture psABI documents. 
>>> Unfortunately, many of them were written before the 'long long'
>>> type became part of the C standard, so it's not strictly guaranteed.
>>> AFAICT, the alignment of __u64 on x86 is the same as the alignment
>>> of 'double' by convention.
>>>
>>> However, the problem is well-understood: x86 is the only one
>>> that has a problem in 32/64 bit compat mode. m68k has similar
>>> issues with 16/32 bit integers, but those don't apply here.
>>>
>>>> If some (perhaps non-gcc) compiler were to lay this out differently
>>>> (perhaps with suitable command-line options) then that's liveable
>>>> with - as long as the kernel never changes the layout.  Of course
>>>> it would be better to avoid this if poss.
>>> If a compiler was using irregular structure alignment, all sorts of
>>> library interfaces would break. The kernel ABI is only a small part
>>> of the problem then.
>>>
>>>> The other potential issue with a structure like this is that there's a
>>>> risk that it will lead us to copy four bytes of uninitialised kernel
>>>> memory out to userspace.
>>>>
>>>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>>>> for this sort of thing.
>>> Agreed in general, but the whole point of this particular patch was to
(Continue reading)

Permalink | Reply | 0 comments |
headers
Geert Uytterhoeven | 1 Feb 2009 11:59

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Geert Uytterhoeven wrote:
> > On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> >> Arnd Bergmann wrote:
> >>> +struct space_resv {
> >>> +	__s16		l_type;
> >>> +	__s16		l_whence;
> >>> +	__s64		l_start;
> >>> +	__s64		l_len;		/* len == 0 means until end of file */
> >>> +	__s32		l_sysid;
> >>> +	__u32		l_pid;
> >>> +	__s32		l_pad[4];	/* reserve area			    */
> >>> +};
> >> What about telling the compiler exactly what you said above, just
> >> to be sure we all mean the same thing. (And as documentation for new
> >> comers):
> >>
> >> +struct space_resv_64 {
> >> +	__s16		l_type;
> >> +	__s16		l_whence;
> >> +	__u32		reserved;
> >> +	__s64		l_start;
> >> +	__s64		l_len;		/* len == 0 means until end of file */
> >> +	__s32		l_sysid;
> >> +	__u32		l_pid;
> >> +	__s32		l_pad[4];	/* reserve area			    */
> >> +} __packed;
> > 
> > Because the compiler will assume all fields are always unaligned and will use very
> > suboptimal code to access them?
(Continue reading)

Permalink | Reply | 0 comments |

Gmane