Re: Concerns in kern/subr_kobj.c

[christos%astron.com@localhost (Christos Zoulas)]

In article <Pine.NEB.4.64.1704180636420.367%speedy.whooppee.com@localhost>,
Paul Goyette  <paul%whooppee.com@localhost> wrote:
>-=-=-=-=-=-
>
>On Mon, 17 Apr 2017, Christos Zoulas wrote:
>
>> In article <Pine.NEB.4.64.1704171932080.13026%speedy.whooppee.com@localhost>,
>> Paul Goyette  <paul%whooppee.com@localhost> wrote:
>>> -=-=-=-=-=-
>>>
>>> On Mon, 17 Apr 2017, Paul Goyette wrote:
>>>
>>>> While perusing the code, I noticed some possible issues:
>>>>
>>>> 1. In kobj_unload() the calls to kobj_machdep() for data and rodata
>>>>   sections are conditional on the appropriate ko->ko_xxx_address being
>>>>   non-zero, yet the corresponding call for the text section is
>>>>   unconditional.
>>>>
>>>>   But, from code just a little bit further down, there is a check
>>>>   for the ko->ko_text_address being 0 before calling uvm_km_free()
>>>>   on the text section.
>>>>
>>>>   It seems to me that both calls should be conditional, or both
>>>>   should be unconditional.
>>>>
>>>> 2. In kobj_affix(), the calls to kobj_machdep() for data and rodata
>>>>   sections discard any possible error value from previous sections.
>>>>   So, if kobj_machdep() fails for the text section, but succeeds for
>>>>   the data or rodata sections, the rest of the code is unaware of
>>>>   any error having occurred.
>>>>
>>>>   In particular, the trailing call to kobj_unload() would not happen;
>>>>   instead the code would simply proceed to update the VM protection
>>>>   bits for the text and rodata sections.
>>>
>>> I'm proposing the attached diffs to address these issues.  Does anyone
>>> see any reason why these should not be committed?
>>
>> How about if (error == 0 && kobj->ko_foo_address) to avoid the extra
>> level of indentation?
>
>Yeah, looks better that way.  Revised diffs attached.

>+	if (error == 0 ko->ko_rodata_address != 0) {

&&

christos