Re: [Y2038] [PATCH v2] generic/402: Make timestamp range check conditional
> As a generic helper, this function has a few problems:
> 1. It assumes scratch dev is mounted (and you're not even calling it
> after _scratch_mount)
Ok, this was an oversight. I was using rootfs as scratch and didn't
catch this in my tests.
> 2. The cleanup() hook won't clean loop mnt/dev if interrupted
Agreed. I can add these in.
There are some existing functions in common/rc that do not clean up.
For example _require_scratch_swapfile might leave partial state if interrupted.
> 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
Some generic functions assume many preconditions.
But, if it is preferred to be more self contained, I can model this
after something like _require_scratch_swapfile()
> All this leads me to think that perhaps it would be better off, at least until
> kernel has fsinfo, to keep this entire helper inside generic/402,
> while addressing
> the issues above in the test itself.
> A more generic solution would be harder and IMO and overkill at this point.
With fsinfo as proposed, it would not be possible to tell if fs ranges
are supported without doing the same kind of workaround.
A capability bit could be added to advertise that feature of VFS, or
it might be reasonable to assume it from the mere existence of fsinfo
syscall.
> What do you think?
The following proposed patch uses a local _cleanup handler that can handle this.
I am okay with either approach. I'm not sure which one is preferable
to xfstests maintainers.
Let me know and I can post it as a V3.
-Deepa
From f539005511f3ad83563cabc6d185b2c76ae37dea Mon Sep 17 00:00:00 2001
From: Deepa Dinamani
Date: Sun, 22 Dec 2019 19:18:14 -0800
Subject: [PATCH v3 1/1] generic/402: Make timestamp range check conditional
Addition of fs-specific timestamp range checking was added
in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check
before running the associated test.
ext4 has been chosen to test for the presence of kernel support
for this feature.
Suggested-by: Amir Goldstein
Signed-off-by: Deepa Dinamani
---
common/rc | 50 +++
tests/generic/402 | 12 +++-
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/common/rc b/common/rc
index eeac1355..796052e6 100644
--- a/common/rc
+++ b/common/rc
@@ -1751,17 +1751,18 @@ _require_loop()
#
_require_ext2()
{
+fs=${1:-ext2}
if [ "$HOSTOS" != "Linux" ]
then
-_notrun "This test requires linux for ext2 filesystem support"
+_notrun "This test requires linux for $fs filesystem support"
fi
-modprobe ext2 >/dev/null 2>&1
-if grep ext2 /proc/filesystems >/dev/null 2>&1
+modprobe $fs >/dev/null 2>&1
+if grep $fs /proc/filesystems >/dev/null 2>&1
then
:
else
-_notrun "This test requires ext2 filesystem support"
+_notrun "This test requires $fs filesystem support"
fi
}
@@ -1981,6 +1982,47 @@ _run_aiodio()
return $status
}
+_require_kernel_timestamp_range()
+{
+
+_require_scratch
+_require_loop
+_require_ext2 ext4
+
+# Use a subshell to clean up the nested loop mount
+_cleanup='( umount $LOOP_MNT ; _destroy_loop_device $LOOP_DEV ;
rm -f $LOOP_FILE ; _scratch_unmount )'
+
+_scratch_mkfs >/dev/null
+_scratch_mount
+
+LOOP_FILE=$SCRATCH_MNT/loop_file
+LOOP_MNT=$SCRATCH_MNT/loop_mnt
+
+dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd ||
_fail "loopback prep failed"
+
+# Use ext4 with 128-byte inodes, which do not have room for
extended timestamp
+FSTYP=ext4 MKFS_OPTIONS=-I128 \
+_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"
+
+LOOP_DEV=$(_create_loop_device $LOOP_FILE)
+mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to
create $LOOP_MNT"
+mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 ||
_fail "ext4 mount failed"
+notrun=false
+_check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT}
supports timestamps until 2038" || \
+notrun=true
+
+umount ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "failed to
umount $LOOP_MNT"
+_destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1
+
+_scratch_unmount
+
+_cleanup=:
+
+if $notrun; then
+_notrun "Kernel does not support timestamp limits"
+fi
+}
+
_require_timestamp_range()
{
local device=${1:-$TEST_DEV}
diff --git a/tests/generic/402 b/tests/generic/402
index 0392c258..4288168a 100755
--- a/tests/generic/402
+++ b/tests/generic/402
@@ -16,7 +16,14 @@ echo "QA output created by $seq"
here=`pwd`
tmp=/tmp/$$
status=1# failure is the default!
-trap "exit \$status" 0 1 2 3 15
+_cleanup=:
+trap "eval \$_cleanup; _cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+cd /
+rm -f $tmp.*
+}
# Get standard environment, filters and checks.
. ./common/rc
@@ -30,6 +37,7 @@ rm -f $seqres.full
_supported_fs generic
_supported_os Linux
_requi
Re: [Y2038] [GIT PULL v3 00/27] block, scsi: final compat_ioctl cleanup
Arnd, > If this version seems ok to everyone, please pull into the scsi tree. Looks good to me. Please address Ben's comment and I'll pull it in. -- Martin K. Petersen Oracle Linux Engineering ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [GIT PULL v3 00/27] block, scsi: final compat_ioctl cleanup
On Thu, 2020-01-02 at 15:55 +0100, Arnd Bergmann wrote: [...] > Changes since v2: > - Rebase to v5.5-rc4, which contains the earlier bugfixes > - Fix sr_block_compat_ioctl() error handling bug found by > Ben Hutchings [...] Unfortunately that fix was squashed into "compat_ioctl: move sys_compat_ioctl() to ioctl.c" whereas it belongs in "compat_ioctl: scsi: move ioctl handling into drivers". If you decide to rebase again, you can add my Reviewed-by to all patches. Ben. -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v3 2/2] xfs: quota: move to time64_t interfaces
On Thu, Jan 02, 2020 at 09:40:46PM +0100, Arnd Bergmann wrote:
> As a preparation for removing the 32-bit time_t type and
> all associated interfaces, change xfs to use time64_t and
> ktime_get_real_seconds() for the quota housekeeping.
>
> This avoids one difference between 32-bit and 64-bit kernels,
> raising the theoretical limit for the quota grace period
> to year 2106 on 32-bit instead of year 2038.
>
> Note that common user space tools using the XFS quotactl
> interface instead of the generic one still use the y2038
> dates.
>
> To fix quotas properly, both the on-disk format and user
> space still need to be changed.
>
> Signed-off-by: Arnd Bergmann
> ---
> This has a small conflict against the series at
> https://www.spinics.net/lists/linux-xfs/msg35409.html
> ("xfs: widen timestamps to deal with y2038") which needs
> to be rebased on top of this.
>
> All other changes to remove time_t and get_seconds()
> are now in linux-next, this is one of the last patches
> needed to remove their definitions for v5.6.
>
> If the widened timestamps make it into v5.6, this patch
> can be dropped.
Not likely. :)
I'll give this series a spin,
Reviewed-by: Darrick J. Wong
--D
> ---
> fs/xfs/xfs_dquot.c | 6 +++---
> fs/xfs/xfs_qm.h | 6 +++---
> fs/xfs/xfs_quotaops.c| 6 +++---
> fs/xfs/xfs_trans_dquot.c | 8 +---
> 4 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 2bff21ca9d78..9cfd3209f52b 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -137,7 +137,7 @@ xfs_qm_adjust_dqtimers(
> (d->d_blk_hardlimit &&
>(be64_to_cpu(d->d_bcount) >
> be64_to_cpu(d->d_blk_hardlimit {
> - d->d_btimer = cpu_to_be32(get_seconds() +
> + d->d_btimer = cpu_to_be32(ktime_get_real_seconds() +
> mp->m_quotainfo->qi_btimelimit);
> } else {
> d->d_bwarns = 0;
> @@ -160,7 +160,7 @@ xfs_qm_adjust_dqtimers(
> (d->d_ino_hardlimit &&
>(be64_to_cpu(d->d_icount) >
> be64_to_cpu(d->d_ino_hardlimit {
> - d->d_itimer = cpu_to_be32(get_seconds() +
> + d->d_itimer = cpu_to_be32(ktime_get_real_seconds() +
> mp->m_quotainfo->qi_itimelimit);
> } else {
> d->d_iwarns = 0;
> @@ -183,7 +183,7 @@ xfs_qm_adjust_dqtimers(
> (d->d_rtb_hardlimit &&
>(be64_to_cpu(d->d_rtbcount) >
> be64_to_cpu(d->d_rtb_hardlimit {
> - d->d_rtbtimer = cpu_to_be32(get_seconds() +
> + d->d_rtbtimer = cpu_to_be32(ktime_get_real_seconds() +
> mp->m_quotainfo->qi_rtbtimelimit);
> } else {
> d->d_rtbwarns = 0;
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 7823af39008b..4e57edca8bce 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -64,9 +64,9 @@ struct xfs_quotainfo {
> struct xfs_inode*qi_pquotaip; /* project quota inode */
> struct list_lru qi_lru;
> int qi_dquots;
> - time_t qi_btimelimit; /* limit for blks timer */
> - time_t qi_itimelimit; /* limit for inodes timer */
> - time_t qi_rtbtimelimit;/* limit for rt blks timer */
> + time64_t qi_btimelimit; /* limit for blks timer */
> + time64_t qi_itimelimit; /* limit for inodes timer */
> + time64_t qi_rtbtimelimit;/* limit for rt blks timer */
> xfs_qwarncnt_t qi_bwarnlimit; /* limit for blks warnings */
> xfs_qwarncnt_t qi_iwarnlimit; /* limit for inodes warnings */
> xfs_qwarncnt_t qi_rtbwarnlimit;/* limit for rt blks warnings */
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index c7de17deeae6..38669e827206 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -37,9 +37,9 @@ xfs_qm_fill_state(
> tstate->flags |= QCI_SYSFILE;
> tstate->blocks = ip->i_d.di_nblocks;
> tstate->nextents = ip->i_d.di_nextents;
> - tstate->spc_timelimit = q->qi_btimelimit;
> - tstate->ino_timelimit = q->qi_itimelimit;
> - tstate->rt_spc_timelimit = q->qi_rtbtimelimit;
> + tstate->spc_timelimit = (u32)q->qi_btimelimit;
> + tstate->ino_timelimit = (u32)q->qi_itimelimit;
> + tstate->rt_spc_timelimit = (u32)q->qi_rtbtimelimit;
> tstate->spc_warnlimit = q->qi_bwarnlimit;
> tstate->ino_warnlimit = q->qi_iwarnlimit;
> tstate->rt_spc_warnlimit = q->qi_rtbwarnlimit;
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index a6fe2d8dc40f..d1b9869bc5fa 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -580,7 +580,7 @@ xfs_trans_dq
[Y2038] [PATCH v3 2/2] xfs: quota: move to time64_t interfaces
As a preparation for removing the 32-bit time_t type and
all associated interfaces, change xfs to use time64_t and
ktime_get_real_seconds() for the quota housekeeping.
This avoids one difference between 32-bit and 64-bit kernels,
raising the theoretical limit for the quota grace period
to year 2106 on 32-bit instead of year 2038.
Note that common user space tools using the XFS quotactl
interface instead of the generic one still use the y2038
dates.
To fix quotas properly, both the on-disk format and user
space still need to be changed.
Signed-off-by: Arnd Bergmann
---
This has a small conflict against the series at
https://www.spinics.net/lists/linux-xfs/msg35409.html
("xfs: widen timestamps to deal with y2038") which needs
to be rebased on top of this.
All other changes to remove time_t and get_seconds()
are now in linux-next, this is one of the last patches
needed to remove their definitions for v5.6.
If the widened timestamps make it into v5.6, this patch
can be dropped.
---
fs/xfs/xfs_dquot.c | 6 +++---
fs/xfs/xfs_qm.h | 6 +++---
fs/xfs/xfs_quotaops.c| 6 +++---
fs/xfs/xfs_trans_dquot.c | 8 +---
4 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 2bff21ca9d78..9cfd3209f52b 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -137,7 +137,7 @@ xfs_qm_adjust_dqtimers(
(d->d_blk_hardlimit &&
(be64_to_cpu(d->d_bcount) >
be64_to_cpu(d->d_blk_hardlimit {
- d->d_btimer = cpu_to_be32(get_seconds() +
+ d->d_btimer = cpu_to_be32(ktime_get_real_seconds() +
mp->m_quotainfo->qi_btimelimit);
} else {
d->d_bwarns = 0;
@@ -160,7 +160,7 @@ xfs_qm_adjust_dqtimers(
(d->d_ino_hardlimit &&
(be64_to_cpu(d->d_icount) >
be64_to_cpu(d->d_ino_hardlimit {
- d->d_itimer = cpu_to_be32(get_seconds() +
+ d->d_itimer = cpu_to_be32(ktime_get_real_seconds() +
mp->m_quotainfo->qi_itimelimit);
} else {
d->d_iwarns = 0;
@@ -183,7 +183,7 @@ xfs_qm_adjust_dqtimers(
(d->d_rtb_hardlimit &&
(be64_to_cpu(d->d_rtbcount) >
be64_to_cpu(d->d_rtb_hardlimit {
- d->d_rtbtimer = cpu_to_be32(get_seconds() +
+ d->d_rtbtimer = cpu_to_be32(ktime_get_real_seconds() +
mp->m_quotainfo->qi_rtbtimelimit);
} else {
d->d_rtbwarns = 0;
diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 7823af39008b..4e57edca8bce 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -64,9 +64,9 @@ struct xfs_quotainfo {
struct xfs_inode*qi_pquotaip; /* project quota inode */
struct list_lru qi_lru;
int qi_dquots;
- time_t qi_btimelimit; /* limit for blks timer */
- time_t qi_itimelimit; /* limit for inodes timer */
- time_t qi_rtbtimelimit;/* limit for rt blks timer */
+ time64_t qi_btimelimit; /* limit for blks timer */
+ time64_t qi_itimelimit; /* limit for inodes timer */
+ time64_t qi_rtbtimelimit;/* limit for rt blks timer */
xfs_qwarncnt_t qi_bwarnlimit; /* limit for blks warnings */
xfs_qwarncnt_t qi_iwarnlimit; /* limit for inodes warnings */
xfs_qwarncnt_t qi_rtbwarnlimit;/* limit for rt blks warnings */
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index c7de17deeae6..38669e827206 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -37,9 +37,9 @@ xfs_qm_fill_state(
tstate->flags |= QCI_SYSFILE;
tstate->blocks = ip->i_d.di_nblocks;
tstate->nextents = ip->i_d.di_nextents;
- tstate->spc_timelimit = q->qi_btimelimit;
- tstate->ino_timelimit = q->qi_itimelimit;
- tstate->rt_spc_timelimit = q->qi_rtbtimelimit;
+ tstate->spc_timelimit = (u32)q->qi_btimelimit;
+ tstate->ino_timelimit = (u32)q->qi_itimelimit;
+ tstate->rt_spc_timelimit = (u32)q->qi_rtbtimelimit;
tstate->spc_warnlimit = q->qi_bwarnlimit;
tstate->ino_warnlimit = q->qi_iwarnlimit;
tstate->rt_spc_warnlimit = q->qi_rtbwarnlimit;
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index a6fe2d8dc40f..d1b9869bc5fa 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -580,7 +580,7 @@ xfs_trans_dqresv(
{
xfs_qcnt_t hardlimit;
xfs_qcnt_t softlimit;
- time_t timer;
+ time64_ttimer;
xfs_qwarncnt_t warns;
xfs_qwarncnt_t warnlimit;
[Y2038] [PATCH v3 1/2] xfs: rename compat_time_t to old_time32_t
The compat_time_t type has been removed everywhere else,
as most users rely on old_time32_t for both native and
compat mode handling of 32-bit time_t.
Remove the last one in xfs.
Reviewed-by: Darrick J. Wong
Reviewed-by: Christoph Hellwig
Signed-off-by: Arnd Bergmann
---
As explained in https://www.spinics.net/lists/linux-xfs/msg35524.html
I've dropped the patch "xfs: disallow broken ioctls without
compat-32-bit-time" for this submission but will get to that later
when doing that as a treewide change.
Please apply these two for v5.6 in the meantime so we can kill off
compat_time_t, time_t and get_seconds() for good.
fs/xfs/xfs_ioctl32.c | 2 +-
fs/xfs/xfs_ioctl32.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index bd07a79ca3c0..9ab0263586da 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -108,7 +108,7 @@ xfs_ioctl32_bstime_copyin(
xfs_bstime_t*bstime,
compat_xfs_bstime_t __user *bstime32)
{
- compat_time_t sec32; /* tv_sec differs on 64 vs. 32 */
+ old_time32_tsec32; /* tv_sec differs on 64 vs. 32 */
if (get_user(sec32, &bstime32->tv_sec) ||
get_user(bstime->tv_nsec, &bstime32->tv_nsec))
diff --git a/fs/xfs/xfs_ioctl32.h b/fs/xfs/xfs_ioctl32.h
index 8c7743cd490e..053de7d894cd 100644
--- a/fs/xfs/xfs_ioctl32.h
+++ b/fs/xfs/xfs_ioctl32.h
@@ -32,7 +32,7 @@
#endif
typedef struct compat_xfs_bstime {
- compat_time_t tv_sec; /* seconds */
+ old_time32_ttv_sec; /* seconds */
__s32 tv_nsec;/* and nanoseconds */
} compat_xfs_bstime_t;
--
2.20.0
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
On Thu, Jan 2, 2020 at 10:16 AM Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig wrote:
> > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote:
> > > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> > > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> > > +{
> > > + if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
> > > + return true;
> > > +
> > > + if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> > > + return true;
> > > +
> > > + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> > > + cmd == XFS_IOC_FSBULKSTAT ||
> > > + cmd == XFS_IOC_SWAPEXT)
> > > + return false;
> > > +
> > > + return true;
> >
> > I think the check for the individual command belongs into the callers,
> > which laves us with:
> >
> > static inline bool have_time32(void)
> > {
> > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) ||
> > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall());
> > }
> >
> > and that looks like it should be in a generic helper somewhere.
>
> Yes, makes sense.
>
> I was going for something XFS specific here because XFS is unique in the
> kernel in completely deprecating a set of ioctl commands (replacing
> the old interface with a v5) rather than allowing the user space to be
> compiled with 64-bit time_t.
I tried adding the helper now but ran into a stupid problem: the best
place to put it would be linux/time32.h, but then I have to include
linux/compat.h from there, which in turn pulls in tons of other
headers in any file using linux/time.h.
I considered making it a macro instead, but that's also really ugly.
I now think we should just defer this change until after v5.6, once I
have separated linux/time.h from linux/time32.h.
In the meantime I'll resend the other two patches that I know we
need in v5.6 in order to get there, so Darrick can apply them to his
tree.
Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [GIT PULL v3 00/27] block, scsi: final compat_ioctl cleanup
On Thu, Jan 2, 2020 at 3:56 PM Arnd Bergmann wrote: > > Hi Martin, James, > > If this version seems ok to everyone, please pull into > the scsi tree. It seems I slightly messed up the Cc list here, in case some of you are missing patches, the full series (22 patches, not 27) is mirrored at https://lore.kernel.org/lkml/20200102145552.1853992-1-a...@arndb.de/T/ as well. Arnd ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
On Thu, Jan 02, 2020 at 10:16:21AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig wrote:
> > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote:
> > > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> > > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> > > +{
> > > + if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
> > > + return true;
> > > +
> > > + if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> > > + return true;
> > > +
> > > + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> > > + cmd == XFS_IOC_FSBULKSTAT ||
> > > + cmd == XFS_IOC_SWAPEXT)
> > > + return false;
> > > +
> > > + return true;
> >
> > I think the check for the individual command belongs into the callers,
> > which laves us with:
> >
> > static inline bool have_time32(void)
> > {
> > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) ||
> > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall());
> > }
> >
> > and that looks like it should be in a generic helper somewhere.
>
> Yes, makes sense.
>
> I was going for something XFS specific here because XFS is unique in the
> kernel in completely deprecating a set of ioctl commands (replacing
> the old interface with a v5) rather than allowing the user space to be
> compiled with 64-bit time_t.
>
> If we add a global helper for this, I'd be tempted to also stick a
> WARN_RATELIMIT() in there to give users a better indication of
> what broke after disabling CONFIG_COMPAT_32BIT_TIME.
>
> The same warning would make sense in the system calls, but then
> we have to decide which combinations we want to allow being
> configured at runtime or compile-time.
>
> a) unmodified behavior
> b) just warn but allow
> c) no warning but disallow
> d) warn and disallow
>
> > > if (XFS_FORCED_SHUTDOWN(mp))
> > > return -EIO;
> > >
> > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
> > > struct fd f, tmp;
> > > int error = 0;
> > >
> > > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> > > + error = -EINVAL;
> > > + goto out;
> > > + }
> >
> > And for this one we just have one cmd anyway. But I actually still
> > disagree with the old_time check for this one entirely, as voiced on
> > one of the last iterations. For swapext the time stamp really is
> > only used as a generation counter, so overflows are entirely harmless.
>
> Sorry I missed that comment earlier. I've had a fresh look now, but
> I think we still need to deprecate XFS_IOC_SWAPEXT and add a
> v5 version of it, since the comparison will fail as soon as the range
> of the inode timestamps is extended beyond 2038, otherwise the
> comparison will always be false, or require comparing the truncated
> time values which would add yet another representation.
I prefer we replace the old SWAPEXT with a new version to get rid of
struct xfs_bstat. Though a SWAPEXT_V5 probably only needs to contain
the *stat fields that swapext actually needs to check that the file
hasn't been changed, which would be ino/gen/btime/ctime.
(Maybe I'd add an offset/length too...)
--D
>Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
[Y2038] [GIT PULL v3 00/27] block, scsi: final compat_ioctl cleanup
Hi Martin, James, If this version seems ok to everyone, please pull into the scsi tree. The following changes since commit e42617b825f8073569da76dc4510bfa019b1c35a: Linux 5.5-rc4 (2019-12-08 14:57:55 -0800) are available in the Git repository at: git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git tags/block-ioctl-cleanup-5.6 for you to fetch changes up to d1329555e914109846283e469b5077e7500ecfaf Documentation: document ioctl interfaces better (2019-12-17 22:45:18 +0100) block, scsi: final compat_ioctl cleanup This series concludes the work I did for linux-5.5 on the compat_ioctl() cleanup, killing off fs/compat_ioctl.c and block/compat_ioctl.c by moving everything into drivers. Overall this would be a reduction both in complexity and line count, but as I'm also adding documentation the overall number of lines increases in the end. My plan was originally to keep the SCSI and block parts separate. This did not work easily because of interdependencies: I cannot do the final SCSI cleanup in a good way without first addressing the CDROM ioctls, so this is one series that I hope could be merged through either the block or the scsi git trees, or possibly both if you can pull in the same branch. The series comes in these steps: 1. clean up the sg v3 interface as suggested by Linus. I have talked about this with Doug Gilbert as well, and he would rebase his sg v4 patches on top of "compat: scsi: sg: fix v3 compat read/write interface" 2. Actually moving handlers out of block/compat_ioctl.c and block/scsi_ioctl.c into drivers, mixed in with cleanup patches 3. Document how to do this right. I keep getting asked about this, and it helps to point to some documentation file. The branch is based on another one that fixes a couple of bugs found during the creation of this series. Changes since v2: - Rebase to v5.5-rc4, which contains the earlier bugfixes - Fix sr_block_compat_ioctl() error handling bug found by Ben Hutchings - Fix idecd_locked_compat_ioctl() compat_ptr() bug - Don't try to handle HDIO_DRIVE_TASKFILE in drivers/ide - More documentation improvements Changes since v1: - move out the bugfixes into a branch for itself - clean up scsi sg driver further as suggested by Christoph Hellwig - avoid some ifdefs by moving compat_ptr() out of asm/compat.h - split out the blkdev_compat_ptr_ioctl function; bug spotted by Ben Hutchings - Improve formatting of documentation [1] https://lore.kernel.org/linux-block/20191211204306.1207817-1-a...@arndb.de/T/#m9f89df30565fc66abbded5d01f4db553b16f129f Arnd Bergmann (22): compat: ARM64: always include asm-generic/compat.h compat: provide compat_ptr() on all architectures compat: scsi: sg: fix v3 compat read/write interface compat_ioctl: block: add blkdev_compat_ptr_ioctl compat_ioctl: ubd, aoe: use blkdev_compat_ptr_ioctl compat_ioctl: move CDROM_SEND_PACKET handling into scsi compat_ioctl: move CDROMREADADIO to cdrom.c compat_ioctl: cdrom: handle CDROM_LAST_WRITTEN compat_ioctl: block: handle cdrom compat ioctl in non-cdrom drivers compat_ioctl: add scsi_compat_ioctl compat_ioctl: bsg: add handler compat_ioctl: ide: floppy: add handler compat_ioctl: scsi: move ioctl handling into drivers compat_ioctl: move sys_compat_ioctl() to ioctl.c compat_ioctl: simplify the implementation compat_ioctl: move cdrom commands into cdrom.c compat_ioctl: scsi: handle HDIO commands from drivers compat_ioctl: move HDIO ioctl handling into drivers/ide compat_ioctl: block: move blkdev_compat_ioctl() into ioctl.c compat_ioctl: block: simplify compat_blkpg_ioctl() compat_ioctl: simplify up block/ioctl.c Documentation: document ioctl interfaces better Documentation/core-api/index.rst | 1 + Documentation/core-api/ioctl.rst | 253 +++ arch/arm64/include/asm/compat.h| 22 +- arch/mips/include/asm/compat.h | 18 -- arch/parisc/include/asm/compat.h | 17 - arch/powerpc/include/asm/compat.h | 17 - arch/powerpc/oprofile/backtrace.c | 2 +- arch/s390/include/asm/compat.h | 6 +- arch/sparc/include/asm/compat.h| 17 - arch/um/drivers/ubd_kern.c | 1 + arch/x86/include/asm/compat.h | 17 - block/Makefile | 1 - block/bsg.c| 1 + block/compat_ioctl.c | 427 - block/ioctl.c | 319 ++ block/scsi_ioctl.c | 214 - drivers/ata/libata-scsi.c | 9 + drivers/block/aoe/aoeblk.c | 1 + drivers/block/floppy.c | 3 + drivers/block/paride/pcd.c | 3 + drivers/block/paride/pd.c | 1 + drivers/block/paride/pf.c | 1 + drivers/
Re: [Y2038] [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig wrote:
> On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote:
> > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> > +{
> > + if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
> > + return true;
> > +
> > + if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> > + return true;
> > +
> > + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> > + cmd == XFS_IOC_FSBULKSTAT ||
> > + cmd == XFS_IOC_SWAPEXT)
> > + return false;
> > +
> > + return true;
>
> I think the check for the individual command belongs into the callers,
> which laves us with:
>
> static inline bool have_time32(void)
> {
> return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) ||
> (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall());
> }
>
> and that looks like it should be in a generic helper somewhere.
Yes, makes sense.
I was going for something XFS specific here because XFS is unique in the
kernel in completely deprecating a set of ioctl commands (replacing
the old interface with a v5) rather than allowing the user space to be
compiled with 64-bit time_t.
If we add a global helper for this, I'd be tempted to also stick a
WARN_RATELIMIT() in there to give users a better indication of
what broke after disabling CONFIG_COMPAT_32BIT_TIME.
The same warning would make sense in the system calls, but then
we have to decide which combinations we want to allow being
configured at runtime or compile-time.
a) unmodified behavior
b) just warn but allow
c) no warning but disallow
d) warn and disallow
> > if (XFS_FORCED_SHUTDOWN(mp))
> > return -EIO;
> >
> > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
> > struct fd f, tmp;
> > int error = 0;
> >
> > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> > + error = -EINVAL;
> > + goto out;
> > + }
>
> And for this one we just have one cmd anyway. But I actually still
> disagree with the old_time check for this one entirely, as voiced on
> one of the last iterations. For swapext the time stamp really is
> only used as a generation counter, so overflows are entirely harmless.
Sorry I missed that comment earlier. I've had a fresh look now, but
I think we still need to deprecate XFS_IOC_SWAPEXT and add a
v5 version of it, since the comparison will fail as soon as the range
of the inode timestamps is extended beyond 2038, otherwise the
comparison will always be false, or require comparing the truncated
time values which would add yet another representation.
Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
