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);
