Received-SPF: neutral (google.com: 209.132.180.67 is neither permitted nor denied by best guess record for domain of linux-kernel-owner@vger.kernel.org) client-ip=209.132.180.67;
Subject: Re: [PATCH] iio: buffer: remove null-checks for 'indio_dev->info'
To:     "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>,
        "jic23@kernel.org" <jic23@kernel.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
References: <20200407145918.6833-1-alexandru.ardelean@analog.com>
 <20200412143008.3fa722a9@archlinux>
 <950bed7e142e7aa1cbd74fe15e560fe7d584af95.camel@analog.com>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <1f16c600-0181-438c-8741-7a2438e3209f@metafoo.de>
Date:   Sun, 12 Apr 2020 19:54:30 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <950bed7e142e7aa1cbd74fe15e560fe7d584af95.camel@analog.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 4/12/20 4:31 PM, Ardelean, Alexandru wrote:
> On Sun, 2020-04-12 at 14:30 +0100, Jonathan Cameron wrote:
>> [External]
>>
>> On Tue, 7 Apr 2020 17:59:18 +0300
>> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>>
>>> Checking for 'indio_dev->info' is an impossible condition, since an IIO
>>> device should NOT be able to register without that information.
>>> The iio_device_register() function won't allow an IIO device to register if
>>> 'indio_dev->info' is NULL.
>>>
>>> If that information somehow becomes NULL, then we're likely busted anyway
>>> and we should crash the system, if we haven't already.
>>>
>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>> I'm glad there was a comment in there to remind me of what was going on here.
>>
>> This is the result of an ancient set from (I think) Lars hardening IIO
>> against forced removal.
>>
>> The indio_dev->info == NULL is deliberately set to true by the IIO core
>> during device remove in order to deal with any inflight data.
>>
>> Reference counting should ensure the device doesn't go away but we need
>> to avoid actually doing anything if this occurs.  That pointer was a
>> convenient option to avoid having to add an explicit flag or 'going away'.
>>
>> Now, it's possible we don't need this any more due to other changes but
>> I certainly don't want to remove it without that being very thoroughly
>> verified!
>>
> Agreed.
> Thanks for the info.
>
> Will think about this a bit later.
>
> Thanks
> Alex

I'm pretty sure we need this. It is to handle the case when the device 
is removed while there a still open file handles. This is necessary 
since we can't stop a physical device from being remove.


E.g. thing a USB device, if somebody pulls it out you can't access it 
anymore. This means you can do any USB requests anymore, but there might 
still be an open file handle. So that means even while the physical 
device has already been removed the open file still has a reference to 
the iio_dev, which means it is kept around and it is possible to do 
read(), poll(), ... on it.

In fact as soon as the remove() callback has finished the underlying 
device is gone, even the module that implements the code might have been 
removed. So we must make sure to not call into any of the callbacks 
anymore. For this reason we set info to NULL.


- Lars


>
>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>   drivers/iio/industrialio-buffer.c | 19 +------------------
>>>   1 file changed, 1 insertion(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
>>> buffer.c
>>> index e6fa1a4e135d..c96071bfada8 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev,
>>> struct iio_buffer *buf,
>>>   	size_t avail;
>>>   	int flushed = 0;
>>>   
>>> -	/* wakeup if the device was unregistered */
>> This comment makes it clear this isn't as 'obvious' as it might at first seem
>> ;)
>>
>>> -	if (!indio_dev->info)
>>> -		return true;
>>> -
>>>   	/* drain the buffer if it was disabled */
>>>   	if (!iio_buffer_is_active(buf)) {
>>>   		to_wait = min_t(size_t, to_wait, 1);
>>> @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
>>> __user *buf,
>>>   	size_t to_wait;
>>>   	int ret = 0;
>>>   
>>> -	if (!indio_dev->info)
>>> -		return -ENODEV;
>>> -
>>>   	if (!rb || !rb->access->read)
>>>   		return -EINVAL;
>>>   
>>> @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
>>> __user *buf,
>>>   
>>>   	add_wait_queue(&rb->pollq, &wait);
>>>   	do {
>>> -		if (!indio_dev->info) {
>>> -			ret = -ENODEV;
>>> -			break;
>>> -		}
>>> -
>>>   		if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) {
>>>   			if (signal_pending(current)) {
>>>   				ret = -ERESTARTSYS;
>>> @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp,
>>>   	struct iio_dev *indio_dev = filp->private_data;
>>>   	struct iio_buffer *rb = indio_dev->buffer;
>>>   
>>> -	if (!indio_dev->info || rb == NULL)
>>> +	if (rb == NULL)
>>>   		return 0;
>>>   
>>>   	poll_wait(filp, &rb->pollq, wait);
>>> @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>>>   		goto out_unlock;
>>>   	}
>>>   
>>> -	if (indio_dev->info == NULL) {
>>> -		ret = -ENODEV;
>>> -		goto out_unlock;
>>> -	}
>>> -
>>>   	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>>>   
>>>   out_unlock: