[PATCH] fat: Allow time_offset to be upto 13 hours

[PATCH] fat: Allow time_offset to be upto 13 hours

[Jan Kara]

Currently we limit values of time_offset mount option to be between -12
and 12 hours. However e.g. zone GMT+12 can have a DST correction on top
which makes the total time difference 13 hours. Update the checks in
mount option parsing to allow offset of upto 13 hours.

Reported-by: Volker Kuhlmann <list0570@paradise.net.nz>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fat/inode.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 509411dd3698..2041031ab1c8 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1146,7 +1146,11 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
 		case Opt_time_offset:
 			if (match_int(&args[0], &option))
 				return -EINVAL;
-			if (option < -12 * 60 || option > 12 * 60)
+			/*
+			 * Allow for GMT+-12 zones to have DST like
+			 * corrections
+			 */
+			if (option < -13 * 60 || option > 13 * 60)
 				return -EINVAL;
 			opts->tz_set = 1;
 			opts->time_offset = option;
-- 
2.1.4

Re: [PATCH] fat: Allow time_offset to be upto 13 hours

[OGAWA Hirofumi]

Jan Kara <jack@suse.cz> writes:

> Currently we limit values of time_offset mount option to be between -12
> and 12 hours. However e.g. zone GMT+12 can have a DST correction on top
> which makes the total time difference 13 hours. Update the checks in
> mount option parsing to allow offset of upto 13 hours.

Hmmm, 13 is where come from?

TZ environment (of course, has much more complex format though. see
tzset(3)) offset is +-24 hours (total different can have more than
24). Well, so I feel, more or less, 12 is sane, and also 24 is sane. But
I'm not sure why 13?

Thanks.

> Reported-by: Volker Kuhlmann <list0570@paradise.net.nz>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/fat/inode.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 509411dd3698..2041031ab1c8 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -1146,7 +1146,11 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
>  		case Opt_time_offset:
>  			if (match_int(&args[0], &option))
>  				return -EINVAL;
> -			if (option < -12 * 60 || option > 12 * 60)
> +			/*
> +			 * Allow for GMT+-12 zones to have DST like
> +			 * corrections
> +			 */
> +			if (option < -13 * 60 || option > 13 * 60)
>  				return -EINVAL;
>  			opts->tz_set = 1;
>  			opts->time_offset = option;

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: Allow time_offset to be upto 13 hours

[Volker Kuhlmann]

On Thu, 26 Nov 2015 03:32:52 OGAWA Hirofumi wrote:

> Hmmm, 13 is where come from?
> 
> TZ environment (of course, has much more complex format though. see
> tzset(3)) offset is +-24 hours (total different can have more than
> 24). Well, so I feel, more or less, 12 is sane, and also 24 is sane.

12 IS NOT SANE because it does not allow to apply DST in some parts of 
the world for half of the year!!! (Basic maths like 12 + 1 = 13 must be 
very difficult to understand for a lot of programmers... problem has 
persisted for decades.)

> But I'm not sure why 13?

It's the minimum that is absolutely required to cover the DST offset in 
New Zealand. At the moment. There may be other places around the date 
line for which even that is not sufficient. Someone's been thoughtless 
already (has happened regularly for decades, probably won't be the last 
time either). Fix it properly.

If the kernel uses +-24 in places then please stick to +-24. There may 
be a reason for that not everyone knows about. The purpose of the range 
check is to be a SANITY check, not to tell me, the user, how I have to 
operate my system. If I want 3 minutes, I want 3 minutes, and it's not 
the kernel programmer's job to enforce.

Or do you wish to track the legal time zone AND DST offsets in every 
place in the world, make a list, stick that into the kernel, and 
maintain it? I hope not.

A good limit would be 2 less of whatever fits into 8 bit. Or something 
like that. Like +-120.

> > Reported-by: Volker Kuhlmann <list0570@paradise.net.nz>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > 
> >  fs/fat/inode.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> > index 509411dd3698..2041031ab1c8 100644
> > --- a/fs/fat/inode.c
> > +++ b/fs/fat/inode.c
> > @@ -1146,7 +1146,11 @@ static int parse_options(struct super_block
> > *sb, char *options, int is_vfat,> 
> >  		case Opt_time_offset:
> >  			if (match_int(&args[0], &option))
> >  			
> >  				return -EINVAL;
> > 
> > -			if (option < -12 * 60 || option > 12 * 60)
> > +			/*
> > +			 * Allow for GMT+-12 zones to have DST like
> > +			 * corrections
> > +			 */
> > +			if (option < -13 * 60 || option > 13 * 60)
> > 
> >  				return -EINVAL;
> >  			
> >  			opts->tz_set = 1;
> >  			opts->time_offset = option;
-- 
Volker Kuhlmann
http://volker.top.geek.nz/	Please do not CC list postings to me.

Re: [PATCH] fat: Allow time_offset to be upto 13 hours

[OGAWA Hirofumi]

Volker Kuhlmann <list0570@paradise.net.nz> writes:

>> But I'm not sure why 13?
>
> It's the minimum that is absolutely required to cover the DST offset in 
> New Zealand. At the moment. There may be other places around the date 
> line for which even that is not sufficient. Someone's been thoughtless 
> already (has happened regularly for decades, probably won't be the last 
> time either). Fix it properly.
>
> If the kernel uses +-24 in places then please stick to +-24. There may 
> be a reason for that not everyone knows about. The purpose of the range 
> check is to be a SANITY check, not to tell me, the user, how I have to 
> operate my system. If I want 3 minutes, I want 3 minutes, and it's not 
> the kernel programmer's job to enforce.
>
> Or do you wish to track the legal time zone AND DST offsets in every 
> place in the world, make a list, stick that into the kernel, and 
> maintain it? I hope not.

Actually, it is correct timezone support (for example, daylight).

> A good limit would be 2 less of whatever fits into 8 bit. Or something 
> like that. Like +-120.

I'm not against to change.  But don't want to change this multiple
times, and step by step. It is why I'm asking.

Well so, how does it overs 24 hours (i.e. a day) for sane tz offset
(implementation is just a dumb adjustment of timestamp, but this option
should means tz offset)? (And +-24 hours is used for TZ spec too.)

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: Allow time_offset to be upto 13 hours

[Volker Kuhlmann]

On Thu, 26 Nov 2015 06:52:37 OGAWA Hirofumi wrote:

> > Or do you wish to track the legal time zone AND DST offsets in every
> > place in the world, make a list, stick that into the kernel, and
> > maintain it? I hope not.
> 
> Actually, it is correct timezone support (for example, daylight).

I don't get what you're trying to say here, sorry.

> I'm not against to change.  But don't want to change this multiple
> times, and step by step. It is why I'm asking.

:-) Agreed.

> Well so, how does it overs 24 hours (i.e. a day) for sane tz offset
> (implementation is just a dumb adjustment of timestamp, but this
> option should means tz offset)? (And +-24 hours is used for TZ spec
> too.)

+-24h offset for time zone (incl DST) should be sufficient. 

However, asking the other way round, can someone please explain why 
there needs to be a limit check? Will there be a kernel panic if someone 
sticks in MAXINT? It's not the kernel's responsibility to enforce any 
setting of user time!

Perhaps the limit check should simply be removed? I can see e.g. using 
this to correct for a past error in setting the time for the device the 
vfat filesystem was created on. That would be cool...
Why enforce limits when you don't have to? Just because it fits the 
programmer's limited view of use cases (see 12 + 1 = OOPS)? Or is this a 
security problem? How about +-2years?

Also see https://bugzilla.suse.com/show_bug.cgi?id=912583#c33  and ff

Volker

-- 
Volker Kuhlmann
http://volker.top.geek.nz/	Please do not CC list postings to me.

Re: [PATCH] fat: Allow time_offset to be upto 13 hours

[OGAWA Hirofumi]

Volker Kuhlmann <list0570@paradise.net.nz> writes:

> On Thu, 26 Nov 2015 06:52:37 OGAWA Hirofumi wrote:
>
>> > Or do you wish to track the legal time zone AND DST offsets in every
>> > place in the world, make a list, stick that into the kernel, and
>> > maintain it? I hope not.
>> 
>> Actually, it is correct timezone support (for example, daylight).
>
> I don't get what you're trying to say here, sorry.

Sorry, my bad english. I meant, using timezone database is only way to
support correct timestamp for fat timestamp format.

>> Well so, how does it overs 24 hours (i.e. a day) for sane tz offset
>> (implementation is just a dumb adjustment of timestamp, but this
>> option should means tz offset)? (And +-24 hours is used for TZ spec
>> too.)
>
> +-24h offset for time zone (incl DST) should be sufficient. 

Yes.

> However, asking the other way round, can someone please explain why 
> there needs to be a limit check? Will there be a kernel panic if someone 
> sticks in MAXINT? It's not the kernel's responsibility to enforce any 
> setting of user time!

No strong opinion in my mind. But keeping it sane/predictable value
would be better.

> Perhaps the limit check should simply be removed? I can see e.g. using 
> this to correct for a past error in setting the time for the device the 
> vfat filesystem was created on. That would be cool...
> Why enforce limits when you don't have to? Just because it fits the 
> programmer's limited view of use cases (see 12 + 1 = OOPS)? Or is this a 
> security problem? How about +-2years?

Hm, not sure what usage it is. Why playing with timestamp for now?
utimes(2) (and possibly UTC) simply?

Thanks.

> Also see https://bugzilla.suse.com/show_bug.cgi?id=912583#c33  and ff
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>