| Message ID | 1448466133-2838-1-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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 <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.
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 <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
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;
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(-)