Date: Wed, 17 Aug 2005 00:40:23 GMT From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/84983: udf filesystem: stat-ting files could randomly fail Message-ID: <200508170040.j7H0eNXi022293@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/84983; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: Andriy Gapon <avg@icyb.net.ua>
Cc: FreeBSD-gnats-submit@freebsd.org, freebsd-bugs@freebsd.org
Subject: Re: kern/84983: udf filesystem: stat-ting files could randomly fail
Date: Wed, 17 Aug 2005 10:37:58 +1000 (EST)
On Tue, 16 Aug 2005, Andriy Gapon wrote:
> System:
> FreeBSD 5.4-RELEASE-p3 #3: Sat Jul 9 17:02:15 EEST 2005 i386
>
>> Description:
> Sometimes stat(2) on files located on UDF fs unexpectedly fails, at the same
> time "udf: invalid FID fragment" kernel messages are produced.
> umount+mount in such situation usually cures the problem, but sometimes
> I have to do kldunload udf in between.
> The symptom very much looks like use of unitialized variable/memory.
Indeed.
> Brief search for a culprit made me suspect udf_node.diroff field.
> This is how udf_node structures are allocated:
>
> udf_vfsops.c: struct udf_node *unode;
> ...
> unode = uma_zalloc(udf_zone_node, M_WAITOK);
>
> i.e. there is no M_ZERO while allocating udf_node and diroff field does
> not seem to be initialized explicitely:
>
> $ fgrep diroff *.[ch]
> udf.h: long diroff;
> udf_vnops.c: if (nameiop != LOOKUP || node->diroff == 0 || node->diroff > fsize) {
> udf_vnops.c: offset = node->diroff;
> udf_vnops.c: node->diroff = ds->offset + ds->off;
>
> as you can see diroff could be used before it is assigned and it is used in
> udf_lookup() function as follows:
>
> if (nameiop != LOOKUP || node->diroff == 0 || node->diroff > fsize) {
> offset = 0;
> numdirpasses = 1;
> } else {
> offset = node->diroff;
> numdirpasses = 2;
> nchstats.ncs_2passes++;
> }
>
> lookloop:
> ds = udf_opendir(node, offset, fsize, udfmp);
>
> as you can see, if diroff belongs to interval (0, fsize] and nameiop is LOOKUP,
> then offset variable will be assigned with its (random) value that, in turn,
> would be passed down to udf_opendir and, thus, directory stream would contain
> incorrect (arbitrary) data.
>
>> How-To-Repeat:
> because of probabalistic nature of the bug, there is no definite recipe of
> reproducing it. you could try to do a lot of operations (ls, stat) on files
> on UDF fs and see if eventually directory udf_node will be allocated with
> "good" junk at diroff position.
> for me, this happens from time to time on its own.
>
>> Fix:
>
> if my theory is correct, then the following patch should fix the problem by
> adding proper initialization to diroff field of udf_node:
>
> --- lookup.patch begins here ---
> --- sys/fs/udf/udf_vfsops.c.orig Mon Aug 15 21:06:50 2005
> +++ sys/fs/udf/udf_vfsops.c Mon Aug 15 21:07:07 2005
> @@ -648,6 +648,7 @@
> return (error);
> }
>
> + unode->diroff = 0;
> unode->i_vnode = vp;
> unode->hash_id = ino;
> unode->i_devvp = udfmp->im_devvp;
This is fixed accidentally in in -current.
I sent essentially the above fix together with fixes for many other bugs
in udf to the udf maintainer 4 months ago:
% Date: Tue, 19 Apr 2005 21:24:34 +1000 (EST)
% From: Bruce Evans <bde@zeta.org.au>
% To: scottl@freebsd.org
% Subject: Re: lots of bugs in udf
%
% On Sun, 10 Apr 2005, Bruce Evans wrote:
%
% > ...
% > Here is my work-in-progress patch:
%
% Here is a better patch with some other bugs fixed:
%
% % Index: udf_vfsops.c
% % ===================================================================
% % RCS file: /home/ncvs/src/sys/fs/udf/udf_vfsops.c,v
% % retrieving revision 1.18
% % diff -u -2 -r1.18 udf_vfsops.c
% % --- udf_vfsops.c 23 Jun 2004 19:36:09 -0000 1.18
% % +++ udf_vfsops.c 19 Apr 2005 09:38:36 -0000
% % @@ -654,4 +654,5 @@
% % unode->i_dev = udfmp->im_dev;
% % unode->udfmp = udfmp;
% % + unode->diroff = 0;
% % vp->v_data = unode;
% % VREF(udfmp->im_devvp);
%
% unode->diroff was not initialized, so udf_lookup() tended to use garbage
% offsets and fail after vnodes were recycled.
%
% The second pass in udf_lookup() should hide this problem, but doesn't
% always because the second pass is not attempted after a bad fid. If
% the stale offset is for a file in the same directory, then the fid is
% not bad and the second pass works.
%
% This was fixed as an undocumented side effect of rev.1.28. uma_zalloc()
% now M_ZEROs everything, although this is less needed than before because
% changes associated with rev.1.28 removed fields from the struct, leaving
% only 5 fields so they are very easy to initialized explicitly. A similar
% change was made to ffs. It may be more needed there. I prefer the old
% code.
%
% [... description of other bugs and fixes deleted]
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200508170040.j7H0eNXi022293>