Re: [PATCH v2 21/24] xfs: quota: move to time64_t interfaces

[Arnd Bergmann]

On Mon, Dec 16, 2019 at 5:52 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Fri, Dec 13, 2019 at 10:17 PM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>>
>> Hmm, so one thing that I clean up on the way to bigtime is the total
>> lack of clamping here.  If (for example) it's September 2105 and
>> rtbtimelimit is set to 1 year, this will cause an integer overflow.  The
>> quota timer will be set to 1970 and expire immediately, rather than what
>> I'd consider the best effort of February 2106.

One more hing to note (I will add this to the changelog text) is that on
32-bit architectures, the limit here is y2038, while on 64-bit
architectures it's y2106:

int xfs_trans_dqresv(...)
{
       time_t                  timer; /* signed 'long' */
       timer = be32_to_cpu(dqp->q_core.d_btimer);
       /* get_seconds() returns unsigned long */
      if ((timer != 0 && get_seconds() > timer))
                return -EDQUOT;
}

> I don't think clamping would be good here, that just replaces
> one bug with another at the overflow time. If you would like to
> have something better before this gets extended, I could try to
> come up with a version that converts it to the nearest 64-bit
> timestamp, similar to the way that time_before32() in the kernel
> or the NTP protocol work.
>
> If you think it can get extended properly soon, I'd just leave the
> patch as it is today in order to remove the get_seconds()
> interface for v5.6.

I've tried this now, and but this feels wrong: it adds lots of complexity
for corner cases and is still fragile, e.g. when the time is wrong
during boot before ntp runs. See that patch below for reference.

I also see that quotatool on xfs always uses the old xfs quota
interface, so it already overflows on the user space side. Fixing
this properly seems to be a bigger effort than I was planning for
(on an unpatched 64-bit kernel):

$ sudo quotatool -b    -u  -t 220month  /mnt/tmp -r
$ rm file ; fallocate -l 11M file
$ sudo quotatool -d /mnt/tmp -u arnd
1000 /mnt/tmp 11264 10240 20480 570239975 2 0 00
$ sudo quotatool -b    -u  -t 222month  /mnt/tmp -r
$ rm file ; fallocate -l 11M file
$ sudo quotatool -d /mnt/tmp -u arnd
1000 /mnt/tmp 11264 10240 20480 18446744069990008316 2 0 00

       Arnd

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 9cfd3209f52b..6c9128bb607b 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -98,6 +98,23 @@ xfs_qm_adjust_dqlimits(
                xfs_dquot_set_prealloc_limits(dq);
 }

+static __be32 xfs_quota_timeout32(s64 limit)
+{
+       time64_t now = ktime_get_real_seconds();
+       u32 timeout;
+
+       /* avoid overflows in out-of-range limits */
+       if ((u64)limit > S32_MAX)
+               limit = S32_MAX;
+       timeout = now + limit;
+
+       /* avoid timeout of zero */
+       if (lower_32_bits(timeout) == 0)
+               return cpu_to_be32(1);
+
+       return cpu_to_be32(lower_32_bits(timeout));
+}
+
 /*
  * Check the limits and timers of a dquot and start or reset timers
  * if necessary.
@@ -137,7 +154,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(ktime_get_real_seconds() +
+                       d->d_btimer = xfs_quota_timeout32(
                                        mp->m_quotainfo->qi_btimelimit);
                } else {
                        d->d_bwarns = 0;
@@ -160,7 +177,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(ktime_get_real_seconds() +
+                       d->d_itimer = xfs_quota_timeout32(
                                        mp->m_quotainfo->qi_itimelimit);
                } else {
                        d->d_iwarns = 0;
@@ -183,7 +200,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(ktime_get_real_seconds() +
+                       d->d_rtbtimer = xfs_quota_timeout32(
                                        mp->m_quotainfo->qi_rtbtimelimit);
                } else {
                        d->d_rtbwarns = 0;
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 1ea82764bf89..2087626b4bee 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -601,6 +601,14 @@ xfs_qm_scall_setqlim(
        return error;
 }

+/* Assume timers are within +/- 68 years of current wall clock */
+static time64_t xfs_quota_time32_to_time64(time64_t now, __be32 timer)
+{
+       s32 diff = be32_to_cpu(timer) - lower_32_bits(now);
+
+       return now + diff;
+}
+
 /* Fill out the quota context. */
 static void
 xfs_qm_scall_getquota_fill_qc(
@@ -609,6 +617,8 @@ xfs_qm_scall_getquota_fill_qc(
        const struct xfs_dquot  *dqp,
        struct qc_dqblk         *dst)
 {
+       time64_t now = ktime_get_real_seconds();
+
        memset(dst, 0, sizeof(*dst));
        dst->d_spc_hardlimit =
                XFS_FSB_TO_B(mp, be64_to_cpu(dqp->q_core.d_blk_hardlimit));
@@ -618,8 +628,8 @@ xfs_qm_scall_getquota_fill_qc(
        dst->d_ino_softlimit = be64_to_cpu(dqp->q_core.d_ino_softlimit);
        dst->d_space = XFS_FSB_TO_B(mp, dqp->q_res_bcount);
        dst->d_ino_count = dqp->q_res_icount;
-       dst->d_spc_timer = be32_to_cpu(dqp->q_core.d_btimer);
-       dst->d_ino_timer = be32_to_cpu(dqp->q_core.d_itimer);
+       dst->d_spc_timer = xfs_quota_time32_to_time64(now,
dqp->q_core.d_btimer);
+       dst->d_ino_timer = xfs_quota_time32_to_time64(now,
dqp->q_core.d_itimer);
        dst->d_ino_warns = be16_to_cpu(dqp->q_core.d_iwarns);
        dst->d_spc_warns = be16_to_cpu(dqp->q_core.d_bwarns);
        dst->d_rt_spc_hardlimit =
@@ -627,7 +637,7 @@ xfs_qm_scall_getquota_fill_qc(
        dst->d_rt_spc_softlimit =
                XFS_FSB_TO_B(mp, be64_to_cpu(dqp->q_core.d_rtb_softlimit));
        dst->d_rt_space = XFS_FSB_TO_B(mp, dqp->q_res_rtbcount);
-       dst->d_rt_spc_timer = be32_to_cpu(dqp->q_core.d_rtbtimer);
+       dst->d_rt_spc_timer = xfs_quota_time32_to_time64(now,
dqp->q_core.d_rtbtimer);
        dst->d_rt_spc_warns = be16_to_cpu(dqp->q_core.d_rtbwarns);

        /*
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index d1b9869bc5fa..c75887da6546 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -636,7 +636,8 @@ xfs_trans_dqresv(
                        }
                        if (softlimit && total_count > softlimit) {
                                if ((timer != 0 &&
-                                    ktime_get_real_seconds() > timer) ||
+                                    time_after32(ktime_get_real_seconds(),
+                                                 timer)) ||
                                    (warns != 0 && warns >= warnlimit)) {
                                        xfs_quota_warn(mp, dqp,
                                                       QUOTA_NL_BSOFTLONGWARN);
@@ -664,7 +665,8 @@ xfs_trans_dqresv(
                        }
                        if (softlimit && total_count > softlimit) {
                                if  ((timer != 0 &&
-                                     ktime_get_real_seconds() > timer) ||
+                                    time_after32(ktime_get_real_seconds(),
+                                                 timer)) ||
                                     (warns != 0 && warns >= warnlimit)) {
                                        xfs_quota_warn(mp, dqp,
                                                       QUOTA_NL_ISOFTLONGWARN);