Bug 106255 - [msdosfs] [patch]: correct setting of archive flag
Summary: [msdosfs] [patch]: correct setting of archive flag
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-03 11:10 UTC by Rene Ladan
Modified: 2018-01-03 05:16 UTC (History)
0 users

See Also:


Attachments
file.diff (355 bytes, patch)
2006-12-03 11:10 UTC, Rene Ladan
no flags Details | Diff
msdos.diff (1.67 KB, patch)
2006-12-04 12:26 UTC, Rene Ladan
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rene Ladan 2006-12-03 11:10:10 UTC
The MSDOS file system has an archive bit in the flags field.  This bit roughly corresponds to the archive flag on the UFS file system.  However, it is set the wrong way around: the flag should be set when the bit is present, and cleared when the bit is absent.

Fix: Patch attached with submission follows:
How-To-Repeat: 1. Mount an MSDOS file system with some files marked as archived, and some not.
2. List its files with 'ls -lo'
   The archive flag will turn up inversed.
Comment 1 Bruce Evans 2006-12-04 00:39:32 UTC
On Sun, 3 Dec 2006, Rene Ladan wrote:

>> Description:
> The MSDOS file system has an archive bit in the flags field.  This bit roughly corresponds to the archive flag on the UFS file system.  However, it is set the wrong way around: the flag should be set when the bit is present, and cleared when the bit is absent.

The comment in msdosfs/direntry.h says that ATTR_ARCHIVE means that
the file is new or modified (in other words, not archived), while the
comment in sys/stat.h says that SF_ARCHIVED means that the file is
archived, but I think both mean that it is archived.

> --- msdosfs_vnops.c	Mon Nov  6 14:41:57 2006
> +++ msdosfs_vnops.c.rene	Sun Dec  3 11:58:47 2006
> @@ -352,7 +352,7 @@
> 		vap->va_ctime = vap->va_mtime;
> 	}
> 	vap->va_flags = 0;
> -	if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
> +	if (dep->de_Attributes & ATTR_ARCHIVE)
> 		vap->va_flags |= SF_ARCHIVED;
> 	vap->va_gen = 0;
> 	vap->va_blocksize = pmp->pm_bpcluster;

This only fixes the reporting of the flag.  msdosfs still maintains
the flag perfectly backwards (except DETIMES() is missing setting of
it for for all changes -- I think all changes to metadata except
possibly to atimes should set it to be perfectly backwards and clear
it to be correct).

Grep shows that this flag is negatively useful in FreeBSD.  No file
systems maintain it (except for getting it backwards in msdosfs).
All archiving utilities need to maintain it for it to be useful,
but none except tar even reference it (except possibly indirectly via
strtofflags(3)), and tar seems to just print it (indirectly via a
special version of strtofflags(3)).

Bruce
Comment 2 Rene Ladan 2006-12-04 09:37:37 UTC
Bruce Evans schreef:
> On Sun, 3 Dec 2006, Rene Ladan wrote:
> 
>>> Description:
>> The MSDOS file system has an archive bit in the flags field.  This bit
>> roughly corresponds to the archive flag on the UFS file system. 
>> However, it is set the wrong way around: the flag should be set when
>> the bit is present, and cleared when the bit is absent.
> 
> The comment in msdosfs/direntry.h says that ATTR_ARCHIVE means that
> the file is new or modified (in other words, not archived), while the
> comment in sys/stat.h says that SF_ARCHIVED means that the file is
> archived, but I think both mean that it is archived.
> 
Indeed, the MSDOS archive bit means that the user should archive the file.

>> --- msdosfs_vnops.c    Mon Nov  6 14:41:57 2006
>> +++ msdosfs_vnops.c.rene    Sun Dec  3 11:58:47 2006
>> @@ -352,7 +352,7 @@
>>         vap->va_ctime = vap->va_mtime;
>>     }
>>     vap->va_flags = 0;
>> -    if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
>> +    if (dep->de_Attributes & ATTR_ARCHIVE)
>>         vap->va_flags |= SF_ARCHIVED;
>>     vap->va_gen = 0;
>>     vap->va_blocksize = pmp->pm_bpcluster;
> 
> This only fixes the reporting of the flag.  msdosfs still maintains
> the flag perfectly backwards (except DETIMES() is missing setting of
> it for for all changes -- I think all changes to metadata except
> possibly to atimes should set it to be perfectly backwards and clear
> it to be correct).
> 
Thanks, I'll look into that.

> Grep shows that this flag is negatively useful in FreeBSD.  No file
> systems maintain it (except for getting it backwards in msdosfs).
> All archiving utilities need to maintain it for it to be useful,
> but none except tar even reference it (except possibly indirectly via
> strtofflags(3)), and tar seems to just print it (indirectly via a
> special version of strtofflags(3)).
>
That's up to the utilities.

> Bruce
> 

Regards,
Rene
-- 
GPG fingerprint = E738 5471 D185 7013 0EE0  4FC8 3C1D 6F83 12E1 84F6
(subkeys.pgp.net)

"It won't fit on the line."
		-- me, 2001
Comment 3 Rene Ladan 2006-12-04 12:26:53 UTC
Bruce Evans schreef:
> On Sun, 3 Dec 2006, Rene Ladan wrote:
> 
>>> Description:
>> The MSDOS file system has an archive bit in the flags field.  This bit
>> roughly corresponds to the archive flag on the UFS file system. 
>> However, it is set the wrong way around: the flag should be set when
>> the bit is present, and cleared when the bit is absent.
> 
> The comment in msdosfs/direntry.h says that ATTR_ARCHIVE means that
> the file is new or modified (in other words, not archived), while the
> comment in sys/stat.h says that SF_ARCHIVED means that the file is
> archived, but I think both mean that it is archived.
> 
[patch 1]

> This only fixes the reporting of the flag.  msdosfs still maintains
> the flag perfectly backwards (except DETIMES() is missing setting of
> it for for all changes -- I think all changes to metadata except
> possibly to atimes should set it to be perfectly backwards and clear
> it to be correct).
>
I've attached a new patch which
  * fixes reporting of the flag (as in the previous patch)
  * sets the archive flag in DETIMES() when the file is created
  * cleans up the logic of not supporting setting the archive flag on
directories (chunk 3)
  * does not set the flag when (vap->va_atime.tv_sec != VNOVAL) or
(vap->va_mode != VNOVAL) in msdosfs_setattr()

I think that only userland tools should send a 'clear request', as the
flag only needs to be cleared when the file is backed up.  The kernel
cannot know when a file has been backed up.

Any comments are welcome.

> Bruce
> 

Regards,
Rene
-- 
GPG fingerprint = E738 5471 D185 7013 0EE0  4FC8 3C1D 6F83 12E1 84F6
(subkeys.pgp.net)

"It won't fit on the line."
		-- me, 2001
Comment 4 Bruce Evans 2006-12-05 00:07:36 UTC
On Mon, 4 Dec 2006, Rene Ladan wrote:

> Bruce Evans schreef:
>> On Sun, 3 Dec 2006, Rene Ladan wrote:
>>
>>>> Description:
>>> The MSDOS file system has an archive bit in the flags field.  This bit
>>> roughly corresponds to the archive flag on the UFS file system.
>>> However, it is set the wrong way around: the flag should be set when
>>> the bit is present, and cleared when the bit is absent.
>>
>> The comment in msdosfs/direntry.h says that ATTR_ARCHIVE means that
>> the file is new or modified (in other words, not archived), while the
>> comment in sys/stat.h says that SF_ARCHIVED means that the file is
>> archived, but I think both mean that it is archived.
>>
> Indeed, the MSDOS archive bit means that the user should archive the file.

Not indeed.  The MSDOS archive bit does mean that the file needs to be
archived, but this means that it has the opposite sense to SF_ARCHIVED
and the main bug is in the patch in the PR.

>>> --- msdosfs_vnops.c    Mon Nov  6 14:41:57 2006
>>> +++ msdosfs_vnops.c.rene    Sun Dec  3 11:58:47 2006
>>> @@ -352,7 +352,7 @@
>>>         vap->va_ctime = vap->va_mtime;
>>>     }
>>>     vap->va_flags = 0;
>>> -    if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
>>> +    if (dep->de_Attributes & ATTR_ARCHIVE)
>>>         vap->va_flags |= SF_ARCHIVED;
>>>     vap->va_gen = 0;
>>>     vap->va_blocksize = pmp->pm_bpcluster;
>>
>> This only fixes the reporting of the flag.  msdosfs still maintains
>> the flag perfectly backwards (except DETIMES() is missing setting of
>> it for for all changes -- I think all changes to metadata except
>> possibly to atimes should set it to be perfectly backwards and clear
>> it to be correct).
>>
> Thanks, I'll look into that.

The above actually only breaks the reporting of the flag.  ATTR_ARCHIVE
has the opposite sese to SF_ARCHIVED, so !ATTR_ARCHIVED must be converted
to SF_ARCHIVED as in the original code above, and the reverse conversion
must be done when setting msdosfs attributes from FreeBSD attributes
(as done in msdosfs_setattr()).  At the user level, thus means that
when ls -o displays "attr", it means that the file is archived, but
when an msdosfs's attrib utility displays "A" it means that the file
is not archived.

The main bug here seems to be just for directories.  The archive
attribute doesn't apply for directories, and the above doesn't have
a special case for directories, so the above tests garbage for the
directory case.  The garbage is usually (maybe always?) 0, so
SF_ARVCHIVED is usually set for directories and ls -o displays "attr".
This is consistent with attrib not displaying "A" but strange.

msdosfs_setattr() is more careful with the archive bit for directories,
but still wrong.  It silently ignores attempts to set this bit.  Thus
applications like cp -p succeed where they should fail, masking the
bug in msdosfs_getattr(): msdosfs_getattr() bogusly turns the absence
of an ATTR_ARCHIVE bit into a setting of SF_ARCHIVED; cp -p tries to
preserve this setting but cannot, and the error goes undetected since
msdosfs_setattr() silently ignores it.

More in another reply.

Bruce
Comment 5 Bruce Evans 2006-12-05 01:44:36 UTC
On Mon, 4 Dec 2006, Rene Ladan wrote:

> I've attached a new patch which
>  * fixes reporting of the flag (as in the previous patch)
>  * sets the archive flag in DETIMES() when the file is created
>  * cleans up the logic of not supporting setting the archive flag on
> directories (chunk 3)
>  * does not set the flag when (vap->va_atime.tv_sec != VNOVAL) or
> (vap->va_mode != VNOVAL) in msdosfs_setattr()

The first fix is the one that most needs some directory logic (see a
previous reply).

Wasn't the archive flag already set in SF_ARCHIVED?

I will enclose my old patches for DETIMES() at the end.  I had noticed
that the archive flag shouldn't be set for null changes to the times.
Also, checking for null changes to times is a good optimization in
general since it avoids some disk writes.  ffs should do it too.  The
disk writes are delayed so many of them get coalesced, but even 1
is expensive.  The file time granularity of 1-2 seconds is much longer
than when CPUs were thousands of times slower.  Of course, this
optimization becomes null for ffs if you use the vfs.timestamp_precision
sysctl to set a very small file time granularity.

> I think that only userland tools should send a 'clear request', as the
> flag only needs to be cleared when the file is backed up.  The kernel
> cannot know when a file has been backed up.

Except it should be a set request of SF_ARCHIVED.

% --- msdosfs_vnops.c.orig	Sun Dec  3 20:45:24 2006
% +++ msdosfs_vnops.c	Mon Dec  4 12:43:37 2006
% @@ -354,7 +354,7 @@
%  		vap->va_birthtime.tv_nsec = 0;
%  	}
%  	vap->va_flags = 0;
% -	if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
% +	if (dep->de_Attributes & ATTR_ARCHIVE)
%  		vap->va_flags |= SF_ARCHIVED;
%  	vap->va_gen = 0;
%  	vap->va_blocksize = pmp->pm_bpcluster;

Needs to do nothing for directories and not change for non-directories.

% @@ -431,12 +431,13 @@
%  			if (error)
%  				return (error);
%  		}
% -		if (vap->va_flags & ~SF_ARCHIVED)
% -			return EOPNOTSUPP;

Removing this is just a bug.  It is needed to reject attempts to set
flags other than SF_ARCHIVED.  All such flags are unsupported.

%  		if (vap->va_flags & SF_ARCHIVED)
%  			dep->de_Attributes &= ~ATTR_ARCHIVE;

I think the error return for the directory case belongs here, although
this is inconsistent with the reversal of the flags.  The archive bit
just doesn't exist for directories so attempts to set SF_ARCHIVED ==
clear ATTR_ARCHIVE are nonsense.  More importantly, the raw bit with
the same value as ATTR_ARCHIVE might have a different meaning for
directories, so it shouldn't be cleared blindly.

% -		else if (!(dep->de_Attributes & ATTR_DIRECTORY))
% -			dep->de_Attributes |= ATTR_ARCHIVE;
% +		else
% +			if (dep->de_Attributes & ATTR_DIRECTORY)
% +				return EOPNOTSUPP;
% +			else 
% +				dep->de_Attributes |= ATTR_ARCHIVE;

This has some indentation errors.

The multiple reversals make this error handling very confusing and wrong,
especially for directories:
- ATTR_ARCHIVE should be clear initially.  Reversing the reversal in setattr
   gives SF_ARCHIVED clear for the wrong reason.
- now if we cp -p the direcctory, then we don't try to set SF_ARCHIVED so
   we reach the above.  SF_ARCHIVED clear is interpreted as a request to
   set ATTR_ARCHIVE and rejected.

Untested fixes:

 		if (dep->de_Attributes & ATTR_DIRECTORY) {
 			if (vap->va_flags & SF_ARCHIVED)
 				return (EOPNOTSUPP);
 		} else {
 			if (vap->va_flags & SF_ARCHIVED) {
 				dep->de_Attributes &= ~ATTR_ARCHIVE;
 			else
 				dep->de_Attributes |= ATTR_ARCHIVE;
 		}

%  		dep->de_flag |= DE_MODIFIED;
%  	}
% 
% @@ -506,8 +507,9 @@
%  				dep->de_flag &= ~DE_UPDATE;
%  				timespec2fattime(&vap->va_mtime, 0,
%  				    &dep->de_MDate, &dep->de_MTime, NULL);
% +				dep->de_Attributes |= ATTR_ARCHIVE;
% +				/* only set archive flag when file has changed */

Various style bugs in the comment.

%  			}
% -			dep->de_Attributes |= ATTR_ARCHIVE;
%  			dep->de_flag |= DE_MODIFIED;

Now it doesn't set the archive flag when the atime changes but the mtime
doesn't change.  Is this intentional?  This change has no effect since
all callers set both times and we don't check for null changes to the
times.  Checking for null changes here is not worth it as an optimization,
but probably right for correctness (so as not to set the archive flag for
null changes).

We don't handle null changes for setting attributes in general.  Msdosfs
doesn't have many attributes so the only other problem cases are:
- null changes to the archive bit itself.  Not detecting these is only
   a pessimization.  It doesn't mess up the archive bit itself.
- null changes to the mode.  These force the archive bit on (for
   directories only).

I once made some minor improvements for null changes to attributes in
ffs.  IIRC, they were for chown(), and the result is the following
very delicate handling of null changes:
- permission is required for even null changes of the uid
- no extra permission is required for null changes of the gid (non-null
   changes require certain group permission).  This is what I changed.
- even null changes must update the inode change time (POSIX spec).
   Similarly for other null changes to attributes.  Thus changes to
   attributes can only end up as null if the inode change time update
   is null.
msdosfs doesn't have uids, gids or inode change times, so it cannot be
a POSIX file system and has more possible null changes to attributes.

%  		}
%  	}
% @@ -531,7 +533,6 @@
%  				dep->de_Attributes &= ~ATTR_READONLY;
%  			else
%  				dep->de_Attributes |= ATTR_READONLY;
% -			dep->de_Attributes |= ATTR_ARCHIVE;
%  			dep->de_flag |= DE_MODIFIED;
%  		}
%  	}

This is for chmod().  I don't think this is right.  Changes to the mode
should be backed up.

% --- denode.h.orig	Thu Oct 26 11:21:07 2006
% +++ denode.h	Mon Dec  4 12:35:00 2006
% @@ -239,6 +239,7 @@
%  		timespec2fattime((cre), 0, &(dep)->de_CDate,		\
%  		    &(dep)->de_CTime, &(dep)->de_CHun);			\
%  		    (dep)->de_flag |= DE_MODIFIED;			\
% +		    (dep)->de_Attributes |= ATTR_ARCHIVE;		\
%  	}								\
%  	(dep)->de_flag &= ~(DE_UPDATE | DE_CREATE | DE_ACCESS);		\
%  } while (0)

Hmm, if the setting is forced in DETIMES() then it might cover changes in
msdosfs_setattr(), like updating the inode change time does in ffs. but
the time here is only the creation time -- the archive flag needs to be
set here but it won't cover all changes like the inode change time does.

Here is my old patch for DETIMES():

% Index: denode.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/fs/msdosfs/denode.h,v
% retrieving revision 1.27
% diff -u -2 -r1.27 denode.h
% --- denode.h	26 Dec 2003 17:24:37 -0000	1.27
% +++ denode.h	27 Dec 2003 07:55:25 -0000
% @@ -220,4 +220,5 @@
%  #define	DETIMES(dep, acc, mod, cre) do {				\
%  	if ((dep)->de_flag & DE_UPDATE) { 				\
% +		/* XXX missing optimization for no-change case. */	\
%  		(dep)->de_flag |= DE_MODIFIED;				\
%  		unix2dostime((mod), &(dep)->de_MDate, &(dep)->de_MTime,	\
% @@ -236,13 +237,16 @@
%  			(dep)->de_flag |= DE_MODIFIED;			\
%  			(dep)->de_ADate = adate;			\
% +			/* XXX intentionally don't set ATTR_ARCHIVE. */	\
%  		}							\
%  	}								\
%  	if ((dep)->de_flag & DE_CREATE) {				\
% +		/* XXX missing optimization for no-change case. */	\
%  		unix2dostime((cre), &(dep)->de_CDate, &(dep)->de_CTime,	\
%  		    &(dep)->de_CHun);					\
% -		    (dep)->de_flag |= DE_MODIFIED;			\
% +		(dep)->de_flag |= DE_MODIFIED;				\
% +		(dep)->de_Attributes |= ATTR_ARCHIVE;	 		\
%  	}								\
%  	(dep)->de_flag &= ~(DE_UPDATE | DE_CREATE | DE_ACCESS);		\
% -} while (0);
% +} while (0)
% 
%  /*

It is the same as your patch except for comments and an identation fix.
I didn't understand DE_CREATE when I wrote it.  DE_CREATE is always
set to gether with DE_UPDATE, and unless there is a bug it is only
set when the file is created (it is quite different from an inode
change time).  Thus the above change is null (ATTR_ARCHIVE has already
been set since DE_UPDATE is set).

Bruce
Comment 6 Remko Lodder freebsd_committer 2006-12-29 20:47:44 UTC
Responsible Changed
From-To: freebsd-bugs->trhodes

assign to tom
Comment 7 John Baldwin freebsd_committer freebsd_triage 2013-05-03 16:51:51 UTC
Posssibly this one is relevant to Ken's file flags patches as well.

-- 
John Baldwin
Comment 8 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 07:58:50 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped