lkml.org 
[lkml]   [2015]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota
From
Hi

On Thu, Jul 2, 2015 at 3:45 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Hi David,
>
> Thank you for reviewing and providing comments on these all! I answered below.
>
> On Thu, Jul 02, 2015 at 10:50:47AM +0200, David Herrmann wrote:
>> Hi
>>
>> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>> > 1) Rewrite
>> >
>> > quota->memory + memory > U32_MAX
>> >
>> > as
>> > U32_MAX - quota->memory < memory
>> >
>> > and provide the comment on why we need that check.
>> >
>> > We have no overflow issue in the original expression when size_t is
>> > 32-bit because the previous one (available - quota->memory < memory)
>> > guarantees that quota->memory + memory doesn't exceed `available'
>> > which is <= U32_MAX in that case.
>> >
>> > But lets stay explicit rather than implicit, it would save us from
>> > describing HOW the code works.
>> >
>> > 2) Add WARN_ON when quota->msgs > KDBUS_CONN_MAX_MSGS
>> >
>> > This is somewhat inconsistent, so we need to properly report it.
>>
>> I don't see the purpose of this WARN_ON(). Sure, ">" should never
>> happen, but that doesn't mean we have to add a WARN_ON. I'd just keep
>> the code as it is.
>
> I agree on WARN_ON. The intention of this change was to provide
> consistency. Current code checks for 'quota->msgs > KDBUS_CONN_MAX_MSGS'
> having '>=' test. If this ever happens, it means that we have a bug, but
> silently ignore it.
>
> If we agree that '>' case should never happen, isn't it better to
> place '==' instead of '>=' in the original test?

I don't see why. This code does not care whether quota->msgs is bigger
than MAX_MSGS. Sure, it does not happen in current code, but this
code-path really doesn't care whether that case can happen or not. All
it does, it verify that it is smaller. Hence, we use ">=".

Furthermore, I usually prefer being rather safe than sorry. WARN_ON()s
are usually not free, but ">=" is for free, if we already have a
condition.

[...]
>>
>> > - if (quota->fds + fds < quota->fds ||
>> > - quota->fds + fds > KDBUS_CONN_MAX_FDS_PER_USER)
>> > + if (WARN_ON(quota->fds > KDBUS_CONN_MAX_FDS_PER_USER) ||
>> > + KDBUS_CONN_MAX_FDS_PER_USER - quota->fds < fds)
>> > return -EMFILE;
>>
>> Not sure the WARN_ON is needed, but this one looks fine to me.
>
> I have the same question here as in the first WARN_ON issue above. If we
> drop WARN_ON, shouldn't we drop the whole 'quota->fds >
> KDBUS_CONN_MAX_FDS_PER_USER' test, assuming that it would never happen?
> Because if we drop WARN_ON but leave the test, it would look ambiguous
> as we check for a bug, but do not address it with some bug reporting
> code.

Same as above.

Thanks
David


\
 
 \ /
  Last update: 2015-07-05 12:21    [W:1.020 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site