| Message ID | 20210913131123.500712780@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Sep 14, 2021 at 1:22 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > /* > * Limits for settimeofday(): > @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett > */ > static inline s64 timespec64_to_ns(const struct timespec64 *ts) > { > - /* Prevent multiplication overflow */ > - if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX) > + /* Prevent multiplication overflow / underflow */ > + if (ts->tv_sec >= KTIME_SEC_MAX) > return KTIME_MAX; > > + if (ts->tv_sec <= KTIME_SEC_MIN) > + return KTIME_MIN; > + I just saw this get merged for the stable kernels, and had not seen this when Thomas originally merged it. I can see how this helps the ptp_clock_adjtime() users, but I just double-checked what other callers exist, and I think it introduces a regression in setitimer(), which does nval = timespec64_to_ns(&value->it_value); ninterval = timespec64_to_ns(&value->it_interval); without any further range checking that I could find. Setting timers with negative intervals sounds like a bad idea, and interpreting negative it_value as a past time instead of KTIME_SEC_MAX sounds like an unintended interface change. I haven't done any proper analysis of the changes, so maybe it's all good, but I think we need to double-check this, and possibly revert it from the stable kernels until a final conclusion. Arnd
On Wed, Sep 15, 2021 at 09:00:32PM +0200, Arnd Bergmann wrote: > On Tue, Sep 14, 2021 at 1:22 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > /* > > * Limits for settimeofday(): > > @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett > > */ > > static inline s64 timespec64_to_ns(const struct timespec64 *ts) > > { > > - /* Prevent multiplication overflow */ > > - if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX) > > + /* Prevent multiplication overflow / underflow */ > > + if (ts->tv_sec >= KTIME_SEC_MAX) > > return KTIME_MAX; > > > > + if (ts->tv_sec <= KTIME_SEC_MIN) > > + return KTIME_MIN; > > + > > I just saw this get merged for the stable kernels, and had not seen this when > Thomas originally merged it. > > I can see how this helps the ptp_clock_adjtime() users, but I just > double-checked > what other callers exist, and I think it introduces a regression in setitimer(), > which does > > nval = timespec64_to_ns(&value->it_value); > ninterval = timespec64_to_ns(&value->it_interval); > > without any further range checking that I could find. Setting timers > with negative intervals sounds like a bad idea, and interpreting negative > it_value as a past time instead of KTIME_SEC_MAX sounds like an > unintended interface change. > > I haven't done any proper analysis of the changes, so maybe it's all > good, but I think we need to double-check this, and possibly revert > it from the stable kernels until a final conclusion. I will revert it now from all stable kernels, thanks. greg k-h
On Thu, Sep 16, 2021 at 6:50 PM OPENSOURCE Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com> wrote: > > > I can see how this helps the ptp_clock_adjtime() users, but I just double-checked what > > other callers exist, and I think it introduces a regression in setitimer(), which does > > > > nval = timespec64_to_ns(&value->it_value); > > ninterval = timespec64_to_ns(&value->it_interval); > > > > without any further range checking that I could find. Setting timers with negative intervals > > sounds like a bad idea, and interpreting negative it_value as a past time instead of KTIME_SEC_MAX > > sounds like an unintended interface change. > > Hello Arnd, > > I have looked into this, and it seems like before your > commit bd40a175769d ("y2038: itimer: change implementation to timespec64") > the "clamping and converting to positive ns" was done using timeval_to_ktime() > and ktime_to_ns(). Actually, looking back at this change, I see that there was an explicit timeval_valid() check in get_itimerval(), and this was moved around but is still there, I guess we're good for this syscall, and the user-visible behavior never actually changed. > When Commit c5021b2547ad ( "time: Prevent undefined behaviour in timespec64_to_ns()" ) > put this functionally into timespec64_to_ns(), the patchnotes mentioned the clamping to > KTIME_SEC_MAX, but did not mention the explicit need to return KTIME_SEC_MAX for any > negative input. Right. > Since timespec64_to_ns() is widely used in drivers, where negative nanosecond values are > quite sensible, I propose to view both of the effects I mentioned above as separate functionalities, > > either to be implemented as separate functions in time64.h (named, for example, timespec64_to_ns() > and timespec64_to_positive_ns), I don't mind having the common version work the way it does after your patch, I was only worried about silently changing the behavior for a documented syscall. > or alternatively, since the setitimer() code seems to be the only one not expecting negative nanoseconds > out of timespec64_to_ns() when fed negative input, the clamping of negative nanosecond values > to KTIME_SEC_MAX to be moved into the setitimer() code, and timespec64_to_ns() to be changed > according to the patch I submitted. > > Both of those alternatives seem trivial and I can send in patches for both of them, > but since this is more a matter of style I would like to hear your opinions on this beforehand. It looks like we don't have to do anything for setitimer(), but that was just the first one that I happened to look at. Did you check the other instances to see if anything might be going wrong there? If they are all good, then I have no other concerns and we should probably put your fix back into the stable kernels (Greg has just reverted it after my initial mail). I went through all instances other than the ptp related ones, and I'm pretty confident that they are all good now, in each case either your patch fixes a bug or the value is already known to be positive and it doesn't matter. Are you confident that the ptp instances are all good as well? I did stumble over one small detail: if (ts->tv_sec <= KTIME_SEC_MIN) return KTIME_MIN; I think this is not entirely correct for the case of tv_sec==KTIME_SEC_MIN with a nonzero tv_nsec, as we now round down to the full second. Not sure if that's worth changing, as we also round up for any value between KTIME_SEC_MAX*NSEC_PER_SEC and KTIME_MAX, or between KTIME_MIN and KTIME_SEC_MIN*NSEC_PER_SEC. In practice I guess we care very little about the last nanosecond in the corner cases. Arnd
Arnd, On Wed, Sep 15 2021 at 21:00, Arnd Bergmann wrote: > On Tue, Sep 14, 2021 at 1:22 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> /* >> * Limits for settimeofday(): >> @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett >> */ >> static inline s64 timespec64_to_ns(const struct timespec64 *ts) >> { >> - /* Prevent multiplication overflow */ >> - if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX) >> + /* Prevent multiplication overflow / underflow */ >> + if (ts->tv_sec >= KTIME_SEC_MAX) >> return KTIME_MAX; >> >> + if (ts->tv_sec <= KTIME_SEC_MIN) >> + return KTIME_MIN; >> + > > I just saw this get merged for the stable kernels, and had not seen this when > Thomas originally merged it. > > I can see how this helps the ptp_clock_adjtime() users, but I just > double-checked > what other callers exist, and I think it introduces a regression in setitimer(), > which does > > nval = timespec64_to_ns(&value->it_value); > ninterval = timespec64_to_ns(&value->it_interval); > > without any further range checking that I could find. Setting timers > with negative intervals sounds like a bad idea, and interpreting negative > it_value as a past time instead of KTIME_SEC_MAX sounds like an > unintended interface change. > > I haven't done any proper analysis of the changes, so maybe it's all > good, but I think we need to double-check this, and possibly revert > it from the stable kernels until a final conclusion. I have done the analysis. setitimer() does not have any problem with that simply because it already checks at the call site that the seconds value is > 0 and so do all the other user visible interfaces. See get_itimerval() ... Granted that the kernel internal interfaces do not have those checks, but they already have other safety nets in place to prevent this and I could not identify any callsite which has trouble with that change. If I failed to spot one then what the heck is the problem? It was broken before that change already! I usually spend quite some time on tagging patches for stable and it's annoying me that this patch got reverted while stuff which I explicitely did not tag for stable got backported for whatever reason and completely against the stable rules: 627ef5ae2df8 ("hrtimer: Avoid double reprogramming in __hrtimer_start_range_ns()") What the heck qualifies this to be backported? 1) It's hot of the press and just got merged in the 5.15-rc1 merge window and is not tagged for stable 2) https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html clearly states the rules but obviously our new fangled "AI" driven approach to select patches for stable is blissfully ignorant of these rules. I assume that AI stands for "Artifical Ignorance' here. I already got a private bug report vs. that on 5.10.65. Annoyingly 5.10.5 does not have the issue despite the fact that the resulting diff between those two versions in hrtimer.c is just in comments. Bah! Thanks, tglx
On Thu, Sep 16 2021 at 22:57, Arnd Bergmann wrote: > On Thu, Sep 16, 2021 at 6:50 PM OPENSOURCE Lukas Hannen > <lukas.hannen@opensource.tttech-industrial.com> wrote: > I did stumble over one small detail: > > if (ts->tv_sec <= KTIME_SEC_MIN) > return KTIME_MIN; > > I think this is not entirely correct for the case of tv_sec==KTIME_SEC_MIN > with a nonzero tv_nsec, as we now round down to the full second. Not sure > if that's worth changing, as we also round up for any value between > KTIME_SEC_MAX*NSEC_PER_SEC and KTIME_MAX, or between > KTIME_MIN and KTIME_SEC_MIN*NSEC_PER_SEC. > In practice I guess we care very little about the last nanosecond in the corner > cases. It's completely irrelevant whether the result is off by one second related to the 292 years limit. Thanks, tglx
On Fri, Sep 17 2021 at 00:32, Thomas Gleixner wrote: > I usually spend quite some time on tagging patches for stable and it's > annoying me that this patch got reverted while stuff which I explicitely > did not tag for stable got backported for whatever reason and completely > against the stable rules: > > 627ef5ae2df8 ("hrtimer: Avoid double reprogramming in __hrtimer_start_range_ns()") > > What the heck qualifies this to be backported? > > 1) It's hot of the press and just got merged in the 5.15-rc1 merge > window and is not tagged for stable > > 2) https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > clearly states the rules but obviously our new fangled "AI" driven > approach to select patches for stable is blissfully ignorant of > these rules. I assume that AI stands for "Artifical Ignorance' here. > > I already got a private bug report vs. that on 5.10.65. Annoyingly > 5.10.5 does not have the issue despite the fact that the resulting diff 5.14.5 obviously... > between those two versions in hrtimer.c is just in comments. > > Bah! > > Thanks, > > tglx
On Fri, Sep 17, 2021 at 12:32 AM Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, Sep 15 2021 at 21:00, Arnd Bergmann wrote: > > I have done the analysis. setitimer() does not have any problem with > that simply because it already checks at the call site that the seconds > value is > 0 and so do all the other user visible interfaces. See > get_itimerval() ... Right, I now came to the same conclusion after taking a closer look, see my reply from yesterday. > Granted that the kernel internal interfaces do not have those checks, > but they already have other safety nets in place to prevent this and I > could not identify any callsite which has trouble with that change. > > If I failed to spot one then what the heck is the problem? It was broken > before that change already! My bad for the unfortunate timing. When only saw the patch when Greg posted it during the stable review and wasn't completely sure about it at the time, so I was hoping that he could just hold off until you had a chance to reply either saying that you had already checked this case or that it was dangerous, but now it's already reverted. I agree we should put back the fix into all stable kernels. Arnd
On Fri, Sep 17, 2021 at 12:32:17AM +0200, Thomas Gleixner wrote: > Arnd, > > On Wed, Sep 15 2021 at 21:00, Arnd Bergmann wrote: > > On Tue, Sep 14, 2021 at 1:22 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > >> /* > >> * Limits for settimeofday(): > >> @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett > >> */ > >> static inline s64 timespec64_to_ns(const struct timespec64 *ts) > >> { > >> - /* Prevent multiplication overflow */ > >> - if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX) > >> + /* Prevent multiplication overflow / underflow */ > >> + if (ts->tv_sec >= KTIME_SEC_MAX) > >> return KTIME_MAX; > >> > >> + if (ts->tv_sec <= KTIME_SEC_MIN) > >> + return KTIME_MIN; > >> + > > > > I just saw this get merged for the stable kernels, and had not seen this when > > Thomas originally merged it. > > > > I can see how this helps the ptp_clock_adjtime() users, but I just > > double-checked > > what other callers exist, and I think it introduces a regression in setitimer(), > > which does > > > > nval = timespec64_to_ns(&value->it_value); > > ninterval = timespec64_to_ns(&value->it_interval); > > > > without any further range checking that I could find. Setting timers > > with negative intervals sounds like a bad idea, and interpreting negative > > it_value as a past time instead of KTIME_SEC_MAX sounds like an > > unintended interface change. > > > > I haven't done any proper analysis of the changes, so maybe it's all > > good, but I think we need to double-check this, and possibly revert > > it from the stable kernels until a final conclusion. > > I have done the analysis. setitimer() does not have any problem with > that simply because it already checks at the call site that the seconds > value is > 0 and so do all the other user visible interfaces. See > get_itimerval() ... > > Granted that the kernel internal interfaces do not have those checks, > but they already have other safety nets in place to prevent this and I > could not identify any callsite which has trouble with that change. > > If I failed to spot one then what the heck is the problem? It was broken > before that change already! > > I usually spend quite some time on tagging patches for stable and it's > annoying me that this patch got reverted while stuff which I explicitely > did not tag for stable got backported for whatever reason and completely > against the stable rules: > > 627ef5ae2df8 ("hrtimer: Avoid double reprogramming in __hrtimer_start_range_ns()") > > What the heck qualifies this to be backported? > > 1) It's hot of the press and just got merged in the 5.15-rc1 merge > window and is not tagged for stable > > 2) https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > clearly states the rules but obviously our new fangled "AI" driven > approach to select patches for stable is blissfully ignorant of > these rules. I assume that AI stands for "Artifical Ignorance' here. > > I already got a private bug report vs. that on 5.10.65. Annoyingly > 5.10.5 does not have the issue despite the fact that the resulting diff > between those two versions in hrtimer.c is just in comments. Looks like Sasha picked it up with the AUTOSEL process, and emailed you about this on Sep 5: https://lore.kernel.org/r/20210906012153.929962-12-sashal@kernel.org I will revert it if you don't think it should be in the stable trees. Also, if you want AUTOSEL to not look at any hrtimer.c patches, just let us know and Sasha will add it to the ignore-list. thanks, greg k-h
On Fri, Sep 17, 2021 at 09:31:33AM +0200, Arnd Bergmann wrote: > On Fri, Sep 17, 2021 at 12:32 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Wed, Sep 15 2021 at 21:00, Arnd Bergmann wrote: > > > > I have done the analysis. setitimer() does not have any problem with > > that simply because it already checks at the call site that the seconds > > value is > 0 and so do all the other user visible interfaces. See > > get_itimerval() ... > > Right, I now came to the same conclusion after taking a closer look, > see my reply from yesterday. > > > Granted that the kernel internal interfaces do not have those checks, > > but they already have other safety nets in place to prevent this and I > > could not identify any callsite which has trouble with that change. > > > > If I failed to spot one then what the heck is the problem? It was broken > > before that change already! > > My bad for the unfortunate timing. When only saw the patch when Greg > posted it during the stable review and wasn't completely sure about it > at the time, so I was hoping that he could just hold off until you had a chance > to reply either saying that you had already checked this case or that it > was dangerous, but now it's already reverted. > > I agree we should put back the fix into all stable kernels. Ok, I'll queue it up again after this round goes out. thanks for the additional review. greg k-h
On Fri, Sep 17 2021 at 10:25, Greg Kroah-Hartman wrote: > On Fri, Sep 17, 2021 at 12:32:17AM +0200, Thomas Gleixner wrote: >> I already got a private bug report vs. that on 5.10.65. Annoyingly >> 5.10.5 does not have the issue despite the fact that the resulting diff >> between those two versions in hrtimer.c is just in comments. The bug report turned out to be a red hering. Probably caused by a bisect gone wrong. The real culprit was the posix-cpu-timer change which got reverted already. > Looks like Sasha picked it up with the AUTOSEL process, and emailed you > about this on Sep 5: > https://lore.kernel.org/r/20210906012153.929962-12-sashal@kernel.org which I obviously missed. > I will revert it if you don't think it should be in the stable trees. It's a pure performance improvement, so according to stable rules it should not be there. > Also, if you want AUTOSEL to not look at any hrtimer.c patches, just let > us know and Sasha will add it to the ignore-list. Nah. I try to pay more attention. I'm not against AUTOSEL per se, but could we change the rules slightly? Any change which is selected by AUTOSEL and lacks a Cc: stable@... is put on hold until acked by the maintainer unless it is a prerequisite for applying a stable tagged fix? This can be default off and made effective on maintainer request. Hmm? Thanks, tglx
On Fri, Sep 17, 2021 at 12:38:43PM +0200, Thomas Gleixner wrote: > On Fri, Sep 17 2021 at 10:25, Greg Kroah-Hartman wrote: > > On Fri, Sep 17, 2021 at 12:32:17AM +0200, Thomas Gleixner wrote: > >> I already got a private bug report vs. that on 5.10.65. Annoyingly > >> 5.10.5 does not have the issue despite the fact that the resulting diff > >> between those two versions in hrtimer.c is just in comments. > > The bug report turned out to be a red hering. Probably caused by a > bisect gone wrong. The real culprit was the posix-cpu-timer change which > got reverted already. > > > Looks like Sasha picked it up with the AUTOSEL process, and emailed you > > about this on Sep 5: > > https://lore.kernel.org/r/20210906012153.929962-12-sashal@kernel.org > > which I obviously missed. > > > I will revert it if you don't think it should be in the stable trees. > > It's a pure performance improvement, so according to stable rules it > should not be there. > > > Also, if you want AUTOSEL to not look at any hrtimer.c patches, just let > > us know and Sasha will add it to the ignore-list. > > Nah. I try to pay more attention. I'm not against AUTOSEL per se, but > could we change the rules slightly? > > Any change which is selected by AUTOSEL and lacks a Cc: stable@... is > put on hold until acked by the maintainer unless it is a prerequisite > for applying a stable tagged fix? > > This can be default off and made effective on maintainer request. > > Hmm? The whole point of the AUTOSEL patches are for the huge numbers of subsystems where maintainers and developers do not care about the stable trees at all, and so they do not mark patches to be backported. So requireing an opt-in like this would defeat the purpose. We do allow the ability to take files/subsystems out of the AUTOSEL process as there are many maintainers that do do this right and get annoyed when patches are picked that they feel shouldn't have. That's the best thing we can do for stuff like this. thanks, greg k-h
Greg, On Fri, Sep 17 2021 at 17:20, Greg Kroah-Hartman wrote: > On Fri, Sep 17, 2021 at 12:38:43PM +0200, Thomas Gleixner wrote: >> Nah. I try to pay more attention. I'm not against AUTOSEL per se, but >> could we change the rules slightly? >> >> Any change which is selected by AUTOSEL and lacks a Cc: stable@... is >> put on hold until acked by the maintainer unless it is a prerequisite >> for applying a stable tagged fix? >> >> This can be default off and made effective on maintainer request. >> >> Hmm? > > The whole point of the AUTOSEL patches are for the huge numbers of > subsystems where maintainers and developers do not care about the stable > trees at all, and so they do not mark patches to be backported. So > requireing an opt-in like this would defeat the purpose. > > We do allow the ability to take files/subsystems out of the AUTOSEL > process as there are many maintainers that do do this right and get > annoyed when patches are picked that they feel shouldn't have. That's > the best thing we can do for stuff like this. I guess I was not able to express myself correctly. What I wanted to say is: 1) Default is AUTOSEL 2) Maintainer can take files/subsystems out of AUTOSEL completely Exists today 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems without a stable tag requires an explicit ACK from the maintainer for the backport. Is new and I would be the first to opt-in :) My rationale for #3 is that even when being careful about stable tags, it happens that one is missing. Occasionaly AUTOSEL finds one of those in my subsystems which I appreciate. Does that make more sense now? Thanks, tglx
On Fri, Sep 17, 2021 at 09:29:32PM +0200, Thomas Gleixner wrote: > Greg, > > On Fri, Sep 17 2021 at 17:20, Greg Kroah-Hartman wrote: > > On Fri, Sep 17, 2021 at 12:38:43PM +0200, Thomas Gleixner wrote: > >> Nah. I try to pay more attention. I'm not against AUTOSEL per se, but > >> could we change the rules slightly? > >> > >> Any change which is selected by AUTOSEL and lacks a Cc: stable@... is > >> put on hold until acked by the maintainer unless it is a prerequisite > >> for applying a stable tagged fix? > >> > >> This can be default off and made effective on maintainer request. > >> > >> Hmm? > > > > The whole point of the AUTOSEL patches are for the huge numbers of > > subsystems where maintainers and developers do not care about the stable > > trees at all, and so they do not mark patches to be backported. So > > requireing an opt-in like this would defeat the purpose. > > > > We do allow the ability to take files/subsystems out of the AUTOSEL > > process as there are many maintainers that do do this right and get > > annoyed when patches are picked that they feel shouldn't have. That's > > the best thing we can do for stuff like this. > > I guess I was not able to express myself correctly. What I wanted to say > is: > > 1) Default is AUTOSEL > > 2) Maintainer can take files/subsystems out of AUTOSEL completely > > Exists today > > 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems > without a stable tag requires an explicit ACK from the maintainer > for the backport. > > Is new and I would be the first to opt-in :) > > My rationale for #3 is that even when being careful about stable tags, > it happens that one is missing. Occasionaly AUTOSEL finds one of those > in my subsystems which I appreciate. > > Does that make more sense now? Ah, yes, that makes much more sense, sorry for the confusion. Sasha, what do you think? You are the one that scripts all of this, not me :) greg k-h
On Sat, Sep 18, 2021 at 05:46:57PM +0200, Greg Kroah-Hartman wrote: >On Fri, Sep 17, 2021 at 09:29:32PM +0200, Thomas Gleixner wrote: >> Greg, >> >> On Fri, Sep 17 2021 at 17:20, Greg Kroah-Hartman wrote: >> > On Fri, Sep 17, 2021 at 12:38:43PM +0200, Thomas Gleixner wrote: >> >> Nah. I try to pay more attention. I'm not against AUTOSEL per se, but >> >> could we change the rules slightly? >> >> >> >> Any change which is selected by AUTOSEL and lacks a Cc: stable@... is >> >> put on hold until acked by the maintainer unless it is a prerequisite >> >> for applying a stable tagged fix? >> >> >> >> This can be default off and made effective on maintainer request. >> >> >> >> Hmm? >> > >> > The whole point of the AUTOSEL patches are for the huge numbers of >> > subsystems where maintainers and developers do not care about the stable >> > trees at all, and so they do not mark patches to be backported. So >> > requireing an opt-in like this would defeat the purpose. >> > >> > We do allow the ability to take files/subsystems out of the AUTOSEL >> > process as there are many maintainers that do do this right and get >> > annoyed when patches are picked that they feel shouldn't have. That's >> > the best thing we can do for stuff like this. >> >> I guess I was not able to express myself correctly. What I wanted to say >> is: >> >> 1) Default is AUTOSEL >> >> 2) Maintainer can take files/subsystems out of AUTOSEL completely >> >> Exists today >> >> 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems >> without a stable tag requires an explicit ACK from the maintainer >> for the backport. >> >> Is new and I would be the first to opt-in :) >> >> My rationale for #3 is that even when being careful about stable tags, >> it happens that one is missing. Occasionaly AUTOSEL finds one of those >> in my subsystems which I appreciate. >> >> Does that make more sense now? > >Ah, yes, that makes much more sense, sorry for the confusion. > >Sasha, what do you think? You are the one that scripts all of this, not >me :) I could give it a go. It adds some complexity here but is probably worth it to avoid issues. Let me think about the best way to go about it. -- Thanks, Sasha
On Mon, Sep 20, 2021 at 10:31:08AM -0400, Sasha Levin wrote: >On Sat, Sep 18, 2021 at 05:46:57PM +0200, Greg Kroah-Hartman wrote: >>On Fri, Sep 17, 2021 at 09:29:32PM +0200, Thomas Gleixner wrote: >>>Greg, >>> >>>On Fri, Sep 17 2021 at 17:20, Greg Kroah-Hartman wrote: >>>> On Fri, Sep 17, 2021 at 12:38:43PM +0200, Thomas Gleixner wrote: >>>>> Nah. I try to pay more attention. I'm not against AUTOSEL per se, but >>>>> could we change the rules slightly? >>>>> >>>>> Any change which is selected by AUTOSEL and lacks a Cc: stable@... is >>>>> put on hold until acked by the maintainer unless it is a prerequisite >>>>> for applying a stable tagged fix? >>>>> >>>>> This can be default off and made effective on maintainer request. >>>>> >>>>> Hmm? >>>> >>>> The whole point of the AUTOSEL patches are for the huge numbers of >>>> subsystems where maintainers and developers do not care about the stable >>>> trees at all, and so they do not mark patches to be backported. So >>>> requireing an opt-in like this would defeat the purpose. >>>> >>>> We do allow the ability to take files/subsystems out of the AUTOSEL >>>> process as there are many maintainers that do do this right and get >>>> annoyed when patches are picked that they feel shouldn't have. That's >>>> the best thing we can do for stuff like this. >>> >>>I guess I was not able to express myself correctly. What I wanted to say >>>is: >>> >>> 1) Default is AUTOSEL >>> >>> 2) Maintainer can take files/subsystems out of AUTOSEL completely >>> >>> Exists today >>> >>> 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems >>> without a stable tag requires an explicit ACK from the maintainer >>> for the backport. >>> >>> Is new and I would be the first to opt-in :) >>> >>>My rationale for #3 is that even when being careful about stable tags, >>>it happens that one is missing. Occasionaly AUTOSEL finds one of those >>>in my subsystems which I appreciate. >>> >>>Does that make more sense now? >> >>Ah, yes, that makes much more sense, sorry for the confusion. >> >>Sasha, what do you think? You are the one that scripts all of this, not >>me :) > >I could give it a go. It adds some complexity here but is probably worth >it to avoid issues. > >Let me think about the best way to go about it. So I'm thinking of yet another patch series that would go out, but instead of AUTOSEL it'll be tagged with "MANUALSEL". It would work the exact same way as AUTOSEL, without the final step of queueing up the commits into the stable trees. Thomas, do you want to give it a go? Want to describe how I filter for commits you'd be taking care of? In the past I'd grep a combo of paths and committers (i.e. net/ && davem@), but you have your hands in too many things :) -- Thanks, Sasha
On Tue, Sep 21 2021 at 15:20, Sasha Levin wrote: > On Mon, Sep 20, 2021 at 10:31:08AM -0400, Sasha Levin wrote: >>On Sat, Sep 18, 2021 at 05:46:57PM +0200, Greg Kroah-Hartman wrote: >>>On Fri, Sep 17, 2021 at 09:29:32PM +0200, Thomas Gleixner wrote: >>>> >>>>I guess I was not able to express myself correctly. What I wanted to say >>>>is: >>>> >>>> 1) Default is AUTOSEL >>>> >>>> 2) Maintainer can take files/subsystems out of AUTOSEL completely >>>> >>>> Exists today >>>> >>>> 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems >>>> without a stable tag requires an explicit ACK from the maintainer >>>> for the backport. >>>> >>>> Is new and I would be the first to opt-in :) >>>> >>>>My rationale for #3 is that even when being careful about stable tags, >>>>it happens that one is missing. Occasionaly AUTOSEL finds one of those >>>>in my subsystems which I appreciate. >>>> >>>>Does that make more sense now? >>> >>>Ah, yes, that makes much more sense, sorry for the confusion. >>> >>>Sasha, what do you think? You are the one that scripts all of this, not >>>me :) >> >>I could give it a go. It adds some complexity here but is probably worth >>it to avoid issues. >> >>Let me think about the best way to go about it. > > So I'm thinking of yet another patch series that would go out, but > instead of AUTOSEL it'll be tagged with "MANUALSEL". It would work the > exact same way as AUTOSEL, without the final step of queueing up the > commits into the stable trees. > > Thomas, do you want to give it a go? Want to describe how I filter for > commits you'd be taking care of? In the past I'd grep a combo of paths > and committers (i.e. net/ && davem@), but you have your hands in too > many things :) Indeed. :( So pretty much all what matches in MAINTAINERS entries where my name happened to end up for some reasons. That would be a good start. Might be a bit overbroad as it also includes x86/kvm, x86/xen, x86/pci which I'm not that involved with, but to make it simple for you, I just volunteered the relevant maintainers (CCed) to participate in that experiment. :) Thanks, tglx
On 21/09/21 22:27, Thomas Gleixner wrote: > 3) Maintainer allows AUTOSEL, but anything picked from files/subsystems > without a stable tag requires an explicit ACK from the maintainer > for the backport. > > Is new and I would be the first to opt-in:) > > My rationale for #3 is that even when being careful about stable tags, > it happens that one is missing. Occasionaly AUTOSEL finds one of those > in my subsystems which I appreciate. > > Does that make more sense now? I like this! > Might be a bit overbroad as it also includes x86/kvm, x86/xen, x86/pci > which I'm not that involved with, but to make it simple for you, I just > volunteered the relevant maintainers (CCed) to participate in that > experiment. I would opt in to MANUALSEL too; so Sasha, feel free to do the same for everything that I'm involved in by the MAINTAINERS file, or we can start with x86/kvm via the generic x86 entries. Thanks, Paolo
--- a/include/linux/time64.h +++ b/include/linux/time64.h @@ -25,7 +25,9 @@ struct itimerspec64 { #define TIME64_MIN (-TIME64_MAX - 1) #define KTIME_MAX ((s64)~((u64)1 << 63)) +#define KTIME_MIN (-KTIME_MAX - 1) #define KTIME_SEC_MAX (KTIME_MAX / NSEC_PER_SEC) +#define KTIME_SEC_MIN (KTIME_MIN / NSEC_PER_SEC) /* * Limits for settimeofday(): @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett */ static inline s64 timespec64_to_ns(const struct timespec64 *ts) { - /* Prevent multiplication overflow */ - if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX) + /* Prevent multiplication overflow / underflow */ + if (ts->tv_sec >= KTIME_SEC_MAX) return KTIME_MAX; + if (ts->tv_sec <= KTIME_SEC_MIN) + return KTIME_MIN; + return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec; }