Page MenuHomeFreeBSD

Prevent overflow issues in timeout processing
ClosedPublic

Authored by smh on Nov 13 2014, 10:36 PM.

Details

Summary

Previously, any timeout value for which (timeout * hz) will overflow the signed integer, will give weird results, since callout(9) routines will convert negative values of ticks to '1'. For unsigned integer overflow we will get sufficiently smaller timeout values than expected.

Switch from callout_reset, which requires conversion to int based ticks to callout_reset_sbt to avoid this.

This was based on the original work done by Eygene Ryabinkin <rea@freebsd.org> back in 5 Aug 2011 which used a macro to help avoid the overlow.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

smh updated this revision to Diff 2393.Nov 13 2014, 10:36 PM
smh retitled this revision from to Properly convert msec timeouts to ticks.
smh updated this object.
smh edited the test plan for this revision. (Show Details)
smh updated this object.Nov 13 2014, 10:41 PM
smh added reviewers: mav, davide.
smh updated this object.
mav requested changes to this revision.Nov 13 2014, 10:57 PM
mav edited edge metadata.

I don't like this change.

  1. I think you are going wrong way converting milliseconds to ticks. callout_reset() you are using is indeed a wrapper around callout_reset_sbt(), so you are adding one more conversion on top, that consumes time and increases chances of overflow or precision loss. You should use callout_reset_sbt() directly and avoid any dependency from HZ.
  1. I am not sure we need one more function in callout API. It is already overbloated, and I would not continue that. I don't seem big problem in using callout_reset_sbt(c, SBT_1MS * timeout, 0, ...) directly.
This revision now requires changes to proceed.Nov 13 2014, 10:57 PM
smh updated this revision to Diff 2399.Nov 14 2014, 12:28 PM
smh edited edge metadata.

Switch from callout_reset to callout_reset_sbt as suggested by mav.

This required the asr and iir driver to be updated to callouts from timeouts and the mpt driver needed mpt_wait_req reworking so I'd appreciate feedback on those changes specifically.

smh added a comment.Nov 14 2014, 12:29 PM
In D1157#5, @mav wrote:

I don't like this change.

  1. I think you are going wrong way converting milliseconds to ticks. callout_reset() you are using is indeed a wrapper around callout_reset_sbt(), so you are adding one more conversion on top, that consumes time and increases chances of overflow or precision loss. You should use callout_reset_sbt() directly and avoid any dependency from HZ.
  2. I am not sure we need one more function in callout API. It is already overbloated, and I would not continue that. I don't seem big problem in using callout_reset_sbt(c, SBT_1MS * timeout, 0, ...) directly.

Thanks mav, seems callout_reset_sbt isn't documented in the man page so not a surprise this option was missed when Eygene originally created this patchset. Updated version based on your changes now uploaded.

mav added a comment.Nov 14 2014, 6:56 PM

That looks better, but:

  • You don't need C_HARDCLOCK do be passed everywhere. It was made to mimic old HZ-based behavior, but these cases have nothing to do with HZ at all.
  • jhb@ yesterday updated iir driver to use callout(9), and I guess he may already have a patch for asr.
smh updated this revision to Diff 2410.Nov 14 2014, 11:18 PM
smh edited edge metadata.

Remove C_HARDCLOCK from reset calls as suggested by mav.

smh retitled this revision from Properly convert msec timeouts to ticks to Prevent overflow issues in timeout processing.Nov 14 2014, 11:25 PM
smh updated this object.
mav added a comment.Nov 14 2014, 11:46 PM

One more bug: asr_ccb is a union, not a struct. That is why I think adding callout structure to it won't work.

The rest of the patch (except not applicable now iir) looks fine to me.

davide edited edge metadata.Nov 14 2014, 11:47 PM

This looks good to me modulo mav's last round of comments.

davide accepted this revision.Nov 14 2014, 11:48 PM
davide edited edge metadata.
smh added a comment.Nov 14 2014, 11:58 PM
In D1157#16, @mav wrote:

One more bug: asr_ccb is a union, not a struct. That is why I think adding callout structure to it won't work.
The rest of the patch (except not applicable now iir) looks fine to me.

Yer I kept that in for now but I've pinged jhb to see if he has anything for asr before I look at that.

Thanks for the reviews guys.

smh updated this revision to Diff 2411.Nov 15 2014, 12:45 AM
smh edited edge metadata.

Revert previous changes to asr in favour of safer timeout to tick conversion as jhb is planing to axe asr anyway.

smh added a reviewer: jhb.Nov 15 2014, 12:47 AM
smh added a comment.Nov 18 2014, 12:57 PM

You OK with this latest revision mav?

mav accepted this revision.Nov 19 2014, 3:29 AM
mav edited edge metadata.
This revision is now accepted and ready to land.Nov 19 2014, 3:29 AM
jhb edited edge metadata.Nov 19 2014, 7:19 PM

Hmm, I have a few more pending timeout -> callout conversions I'd like to commit that you might then want to fix. Can you give me another day or so to commit those?

smh added a comment.Nov 19 2014, 7:23 PM

Sure no problem, give me a prod when your done :)

jhb added a comment.Nov 21 2014, 7:12 PM

I'm done with merging anything that affects you. My only remaining patch is to kern_fail.c which you don't touch now.

smh closed this revision.Nov 21 2014, 9:01 PM
smh updated this revision to Diff 2503.

Closed by commit rS274819 (authored by @smh).