Re: [PATCH v3 1/6] vfio-ccw: make it safe to access channel programs

Eric Farman
2019-02-05T14:41:15Z

On 02/05/2019 07:03 AM, Cornelia Huck wrote:
> On Mon, 4 Feb 2019 14:25:34 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
>
>> On 01/30/2019 08:22 AM, Cornelia Huck wrote:
>>> When we get a solicited interrupt, the start function may have
>>> been cleared by a csch, but we still have a channel program
>>> structure allocated. Make it safe to call the cp accessors in
>>> any case, so we can call them unconditionally.
>>>
>>> While at it, also make sure that functions called from other parts
>>> of the code return gracefully if the channel program structure
>>> has not been initialized (even though that is a bug in the caller).
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>> drivers/s390/cio/vfio_ccw_cp.c | 20 +++++++++++++++++++-
>>> drivers/s390/cio/vfio_ccw_cp.h | 2 ++
>>> drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++
>>> 3 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>>> index ba08fe137c2e..0bc0c38edda7 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -335,6 +335,7 @@ static void cp_unpin_free(struct channel_program *cp)
>>> struct ccwchain *chain, *temp;
>>> int i;
>>>
>>> + cp->initialized = false;
>>> list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
>>> for (i = 0; i < chain->ch_len; i++) {
>>> pfn_array_table_unpin_free(chain->ch_pat + i,
>>> @@ -701,6 +702,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>> */
>>> cp->orb.cmd.c64 = 1;
>>>
>>> + cp->initialized = true;
>>> +
>>
>> Not seen in this hunk, but we call ccwchain_loop_tic() just prior to
>> this point. If that returns non-zero, we call cp_unpin_free()[1] (and
>> set initailized to false), and then fall through to here. So this is
>> going to set initialized to true, even though we're taking an error
>> path. :-(
>
> Eek, setting c64 unconditionally threw me off. This needs to check
> for !ret, of course.
>
>>
>> [1] Wait, why is it calling cp_unpin_free()? Oh, I had proposed
>> squashing cp_free() and cp_unpin_free() back in November[2], got an r-b
>> from Pierre but haven't gotten back to tidy up the series for a v2.
>> Okay, I'll try to do that again soon. :-)
>
> :)
>
>> [2] https://patchwork.kernel.org/patch/10675261/
>>
>>> return ret;
>>> }
>>>
>>> @@ -715,7 +718,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>> */
>>> void cp_free(struct channel_program *cp)
>>> {
>>> - cp_unpin_free(cp);
>>> + if (cp->initialized)
>>> + cp_unpin_free(cp);
>>> }
>>>
>>> /**
>>> @@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
>>> struct ccwchain *chain;
>>> int len, idx, ret;
>>>
>>> + /* this is an error in the caller */
>>> + if (!cp || !cp->initialized)
>>> + return -EINVAL;
>>> +
>>> list_for_each_entry(chain, &cp->ccwchain_list, next) {
>>> len = chain->ch_len;
>>> for (idx = 0; idx < len; idx++) {
>>> @@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
>>> struct ccwchain *chain;
>>> struct ccw1 *cpa;
>>>
>>> + /* this is an error in the caller */
>>> + if (!cp || !cp->initialized)
>>> + return NULL;
>>> +
>>> orb = &cp->orb;
>>>
>>> orb->cmd.intparm = intparm;
>>> @@ -831,6 +843,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
>>> u32 cpa = scsw->cmd.cpa;
>>> u32 ccw_head, ccw_tail;
>>>
>>> + if (!cp->initialized)
>>> + return;
>>> +
>>> /*
>>> * LATER:
>>> * For now, only update the cmd.cpa part. We may need to deal with
>>> @@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
>>> struct ccwchain *chain;
>>> int i;
>>>
>>> + if (!cp->initialized)
>>
>> So, two of the checks added above look for a nonzero cp pointer prior to
>> checking initialized, while two don't. I guess cp can't be NULL, since
>> it's embedded in the private struct directly and that's only free'd when
>> we do vfio_ccw_sch_remove() ... But I guess some consistency in how we
>> look would be nice.
>
> The idea was: In which context is this called? Is there a legitimate
> reason for the caller to pass in an uninitialized cp, or would that
> mean the caller had messed up (and we should not trust cp to be !NULL
> either?)
>
> But you're right, that does look inconsistent. Always checking for
> cp != NULL probably looks least odd, although it is overkill. Opinions?

My opinion? Since cp is embedded in vfio_ccw_private, rather than a
pointer to a separately malloc'd struct, we pass &private->cp to those
functions. So a check for !cp doesn't really buy us anything because
what we are actually concerned about is whether or not private is NULL,
which only changes on the probe/remove boundaries.

>
>>
>>> + return false;
>>> +
>>> list_for_each_entry(chain, &cp->ccwchain_list, next) {
>>> for (i = 0; i < chain->ch_len; i++)
>>> if (pfn_array_table_iova_pinned(chain->ch_pat + i,
>