svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/...

[Pedro Giffuni]

On 01/24/18 12:37, Conrad Meyer wrote:
> On Tue, Jan 23, 2018 at 11:40 AM, Pedro Giffuni <pfg at freebsd.org> wrote:
>> On 23/01/2018 14:08, Conrad Meyer wrote:
>>> On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <pfg at freebsd.org> wrote:
>>>> Author: pfg
>>>> Date: Sun Jan 21 15:42:36 2018
>>>> New Revision: 328218
>>> I'm confused about this change.  Wouldn't it be better to remove the
>>> annotation/attributes from mallocarray() than to remove the protection
>>> against overflow?
>>
>> Not in my opinion: it would be better to detect such overflows at compile
>> time (or through a static analyzer) than to have late notification though
>> panics.
> Sure, it would be better, but some situations are only detected at
> runtime -- hence mallocarray.  And occasional use of the annotations
> on systems with plenty of RAM would keep the source tree free of
> compiler-detectable overflows, which I suspect are incredibly
> uncommon.

I think the runtime error cases are way more uncommon, specially if the 
checks are unnecessary.
In any case I collected the patch with malloc--> mallocarray changes in 
PR 225197.
Maybe their are useful with fuzzing.

>> The blind use of mallocarray(9) is probably a mistake also: we
>> shouldn't use it unless there is some real risk of overflow.
> I'm not sure I follow that.

You normally don't get "tainted" values in the kernel. Most of the 
allocations in question were variables with very small number multiplied 
by the size of an int. As Bruce mentioned, we don't do big allocations 
with malloc(9), at least not the size required to get an unsigned number 
overflow. In such cases checking for an overflow is a complete waste of 
time. And then there is also the bug of mallocarray(9) size types not 
matching malloc(9).

>>>     (If the compiler is fixed in the future to not use
>>> excessive memory with these attributes, they can be conditionalized on
>>> compiler version, of course.)
>> All in all, the compiler is not provably wrong: it's just using more swap
>> space, which is rather inconvenient for small platforms but not necessarily
>> wrong.
> Seems wrong if it's a noticeable amount.  Maybe we could flip the
> annotations on or off with a low-ram build knob or something like
> that.

There is actually no proof that this was related to the attributes. I 
absolutely dislike the idea of disabling the attributes and even more 
the idea of adding complex machinery to the system headers to account 
for such case.

Pedro.