From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752505AbcBWLUA (ORCPT ); Tue, 23 Feb 2016 06:20:00 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:57614 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751021AbcBWLT5 (ORCPT ); Tue, 23 Feb 2016 06:19:57 -0500 X-AuditID: cbfee68e-f793c6d00000136c-8b-56cc405b9aea Date: Tue, 23 Feb 2016 11:19:55 +0000 (GMT) From: EunTaik Lee Subject: Re: Re: [PATCH v2] staging/android/ion : fix a race condition in the ion driver To: Laura Abbott , "gregkh@linuxfoundation.org" , "arve@android.com" , "riandrews@android.com" , "sumit.semwal@linaro.org" , "dan.carpenter@oracle.com" , Rohit Kumar , "sriram@marirs.net.in" , "shawn.lin@rock-chips.com" , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" , "euntaik@gmail.com" Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160223111717837@eun.taik.lee Msgkey: 20160223111717837@eun.taik.lee X-EPLocale: ko_KR.euc-kr X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-MLAttribute: X-RootMTR: 20160223111717837@eun.taik.lee X-ParentMTR: X-ArchiveUser: EV X-CPGSPASS: Y X-ConfirmMail: N,general Content-type: text/plain; charset=euc-kr MIME-version: 1.0 Message-id: <2032803520.119281456226390973.JavaMail.weblogic@epmlwas08c> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDJsWRmVeSWpSXmKPExsVy+t8zHd1ohzNhBn8uaFtc3jWHzYHR4/Mm uQDGqAZGm8Si5IzMslSF1Lzk/JTMvHRbpdAQN10LJYWM/OISW6VoIwNjPSNTEz0jE3M9S4NY KyNTJYW8xNxUW6UKXaheJYWi5AKg2tzKYqABOal6UHG94tS8FIes/FKQS/SKE3OLS/PS9ZLz c5UUyhJzSoFGKOknTGXMuH/9LVPBi6qK+Z0bmBoYL5R3MXJyCAmoS5zYvYYFxJYQMJE4eOAF G4QtJnHh3nogmwuoZhmjxJat11hhip7+XsYOkZjDKNHQvgesg0VAVWJJ3wqwSWwCuhL/P3ax g9jCAhESe191MYI0iAi8ZpF429DLDLFaSWL+4QawBl4BQYmTM59AnaEqMWN5KyNEXE1i2bo/ zBBxCYlZ0y9AXcErMaP9KVS9nMS0r2ugaqQlzs/awAjzwuLvj6Hi/BLHbu9ggrAFJKaeOQhV oyVx/uNEqJf5JNYsfAs1U1Di9LVuZphdDRt/s8PcsLXlCdgNzAKKElO6H7JD2FoSX37sY0P3 C6+Ah8SzQw+hdnVySGw+ZQcJLAGJb5MPsUxgVJyFpGUWkrGzkIxFVrOAkWUVo2hqQXJBcVJ6 kRFyfG9ihCTDvh2MNw9YH2IU4GBU4uH19DodJsSaWFZcmXuIMRlo9URmKdHkfGDKzSuJNzQ2 M7IwNTE1NjK3NMMQNjG1sDAxwiGsJM6bIPUzWEggPbEkNTs1tSC1KL6oNCe1+BAjEwenVAPj zMRbW7KyNzjtSwxzim/6IHegc2WTyY5JIjvivh75xyrw7ahR0ev3Zvv2m7oIF252+fBTKd7i f9k+tvm3rp88rbrTeJqXvPDcteu2ZWkyTXSxWTxhnTr7ZcGdlavzuhmWbpwUuvoEx3mbu9e0 924y1VpR/edhFMOh2ObJgg6tXtMY3GO+iLUeVGIpzkg01GIuKk4EAFgSNvGvAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrHKsWRmVeSWpSXmKPExsVy+t/tft1ohzNhBvdb1S0u75rD5sDo8XmT XABjVIZNRmpiSmqRQmpecn5KZl66rZJ3cLxzvKmZgaGuoaWFuZJCXmJuqq2Si0+ArltmDtBQ JYWyxJxSoFBAYnGxkr6dTVF+aUmqQkZ+cYmtUrSRgbGekamJnpGxgZ6JQayVoYGBkSlQVUJG xv3rb5kKXlRVzO/cwNTAeKG8i5GTQ0hAXeLE7jUsILaEgInE09/L2CFsMYkL99azdTFyAdXM YZRoaN/DBpJgEVCVWNK3AqyBTUBX4v/HLrAGYYEIib2vuhhBGkQEXrNIvG3oZYbYoCQx/3AD WAOvgKDEyZlPoLapSsxY3soIEVeTWLbuDzNEXEJi1vQLrBA2r8SM9qdQ9XIS076ugaqRljg/ awMjzKWLvz+GivNLHLu9gwnCFpCYeuYgVI2WxPmPE9kgbD6JNQvfQs0UlDh9rZsZZlfDxt/s MDdsbXkCdgOzgKLElO6H7BC2lsSXH/vY0P3CK+Ah8ezQQ8YJjDKzkKRmIWmfhaQdWc0CRpZV jKKpBckFxUnpFcZ6xYm5xaV56XrJ+bmbGMFp59niHYz/z1sfYhTgYFTi4f3gfjpMiDWxrLgy 9xCjBAezkgjvdZ0zYUK8KYmVValF+fFFpTmpxYcYTYFRNZFZSjQ5H5gS80riDY0NjA0NLc0N TA2NLJTEeQP+rgsTEkhPLEnNTk0tSC2C6WPi4JRqYMzexbZj9rxCBtWNi/btbJXP+OnBepHp BJvYae8vTaxiFgtKdmx2CPlt7x3Q/Oj2Q5nqoN1BX3iq2m9nRvWy5V66ckPlzwarGKvl9hNO n/qk8e3g/Bd+1yslXd893HVDaMWFu5OfnJA+Y3bI/dYDy0Xna9Nn/3j9dWrqEeWDlZe3K3pw nDvumJioxFKckWioxVxUnAgA7JQutVEDAAA= DLP-Filter: Pass X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u1NBK69s009786 > From: Laura Abbott [mailto:labbott@redhat.com] > Sent: Saturday, February 20, 2016 5:09 AM > To: eun.taik.lee@samsung.com; gregkh@linuxfoundation.org; arve@android.com; > riandrews@android.com; sumit.semwal@linaro.org; dan.carpenter@oracle.com; > Rohit Kumar ; sriram@marirs.net.in; shawn.lin@rock- > chips.com; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; > euntaik@gmail.com > Subject: Re: [PATCH v2] staging/android/ion : fix a race condition in the > ion driver > > On 02/19/2016 04:03 AM, EunTaik Lee wrote: > > There is a use-after-free problem in the ion driver. > > This is caused by a race condition in the ion_ioctl() function. > > > > A handle has ref count of 1 and two tasks on different cpus calls > > ION_IOC_FREE simultaneously. > > > > cpu 0 cpu 1 > > ------------------------------------------------------- > > ion_handle_get_by_id() > > (ref == 2) > > ion_handle_get_by_id() > > (ref == 3) > > > > ion_free() > > (ref == 2) > > > > ion_handle_put() > > (ref == 1) > > > > ion_free() > > (ref == 0 so ion_handle_destroy() is > > called > > and the handle is freed.) > > > > ion_handle_put() is called and it > > decreases the slub's next free pointer > > > > The problem is detected as an unaligned access in the spin lock > > functions since it uses load exclusive > > instruction. In some cases it corrupts the slub's free pointer which > > causes a mis-aligned access to the next free pointer.(kmalloc returns > > a pointer like ffffc0745b4580aa). And it causes lots of other > > hard-to-debug problems. > > > > This symptom is caused since the first member in the ion_handle > > structure is the reference count and the ion driver decrements the > > reference after it has been freed. > > > > To fix this problem client->lock mutex is extended to protect all the > > codes that uses the handle. > > > > Signed-off-by: Eun Taik Lee > > --- > > changes in v2 : > > 1. add problem description in the comment > > 2. fix un-matching mutex_lock/unlock pair in ion_share_dma_buf() > > > > drivers/staging/android/ion/ion.c | 102 > ++++++++++++++++++++++++++++++-------- > > 1 file changed, 82 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/staging/android/ion/ion.c > > b/drivers/staging/android/ion/ion.c > > index e237e9f..c6fbe48 100644 > > --- a/drivers/staging/android/ion/ion.c > > +++ b/drivers/staging/android/ion/ion.c > > @@ -385,13 +385,22 @@ static void ion_handle_get(struct ion_handle > *handle) > > kref_get(&handle->ref); > > } > > > > +static int ion_handle_put_nolock(struct ion_handle *handle) { > > + int ret; > > + > > + ret = kref_put(&handle->ref, ion_handle_destroy); > > + > > + return ret; > > +} > > + > > the > > > static int ion_handle_put(struct ion_handle *handle) > > { > > struct ion_client *client = handle->client; > > int ret; > > > > mutex_lock(&client->lock); > > - ret = kref_put(&handle->ref, ion_handle_destroy); > > + ret = ion_handle_put_nolock(handle); > > mutex_unlock(&client->lock); > > > > return ret; > > @@ -415,20 +424,30 @@ static struct ion_handle *ion_handle_lookup(struct > ion_client *client, > > return ERR_PTR(-EINVAL); > > } > > > > -static struct ion_handle *ion_handle_get_by_id(struct ion_client > *client, > > - int id) > > +static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client > *client, > > + int id) > > { > > struct ion_handle *handle; > > > > - mutex_lock(&client->lock); > > handle = idr_find(&client->idr, id); > > if (handle) > > ion_handle_get(handle); > > - mutex_unlock(&client->lock); > > > > return handle ? handle : ERR_PTR(-EINVAL); > > } > > > > +struct ion_handle *ion_handle_get_by_id(struct ion_client *client, > > + int id) > > +{ > > + struct ion_handle *handle; > > + > > + mutex_lock(&client->lock); > > + handle = ion_handle_get_by_id_nolock(client, id); > > + mutex_unlock(&client->lock); > > + > > + return handle; > > +} > > + > > static bool ion_handle_validate(struct ion_client *client, > > struct ion_handle *handle) > > { > > @@ -530,7 +549,8 @@ struct ion_handle *ion_alloc(struct ion_client > *client, size_t len, > > } > > EXPORT_SYMBOL(ion_alloc); > > > > -void ion_free(struct ion_client *client, struct ion_handle *handle) > > +static void ion_free_nolock(struct ion_client *client, > > + struct ion_handle *handle) > > { > > bool valid_handle; > > > > @@ -538,15 +558,24 @@ void ion_free(struct ion_client *client, struct > > ion_handle *handle) > > > > mutex_lock(&client->lock); > > valid_handle = ion_handle_validate(client, handle); > > - > > if (!valid_handle) { > > WARN(1, "%s: invalid handle passed to free.\n", __func__); > > mutex_unlock(&client->lock); > > return; > > } > > + ion_handle_put_nolock(handle); > > +} > > + > > +void ion_free(struct ion_client *client, struct ion_handle *handle) { > > + BUG_ON(client != handle->client); > > + > > + mutex_lock(&client->lock); > > + ion_free_nolock(client, handle); > > mutex_unlock(&client->lock); > > ion_handle_put(handle); > > } > > + > > EXPORT_SYMBOL(ion_free); > > > > This still doesn't look right. ion_handle_put is being called twice on > ion_free, once in ion_free_nolock and once again right after. Please > double check this > Yes, that shouldn't have been there. > > int ion_phys(struct ion_client *client, struct ion_handle *handle, > > @@ -830,6 +859,7 @@ void ion_client_destroy(struct ion_client *client) > > struct rb_node *n; > > > > pr_debug("%s: %d\n", __func__, __LINE__); > > + mutex_lock(&client->lock); > > while ((n = rb_first(&client->handles))) { > > struct ion_handle *handle = rb_entry(n, struct ion_handle, > > node); > > @@ -837,6 +867,7 @@ void ion_client_destroy(struct ion_client *client) > > } > > > > idr_destroy(&client->idr); > > + mutex_unlock(&client->lock); > > > > The mutex_lock here isn't necessary. This is the client destroy and > handles are local to a client so there is nothing to protect here. If > ion_client_destroy is being called on the same client at the same time we > have bigger issues. > > > > down_write(&dev->lock); > > if (client->task) > > @@ -1100,7 +1131,7 @@ static struct dma_buf_ops dma_buf_ops = { > > .kunmap = ion_dma_buf_kunmap, > > }; > > > > -struct dma_buf *ion_share_dma_buf(struct ion_client *client, > > +static struct dma_buf *ion_share_dma_buf_nolock(struct ion_client > > +*client, > > struct ion_handle *handle) > > { > > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > > @@ -1108,7 +1139,6 @@ struct dma_buf *ion_share_dma_buf(struct > ion_client *client, > > struct dma_buf *dmabuf; > > bool valid_handle; > > > > - mutex_lock(&client->lock); > > valid_handle = ion_handle_validate(client, handle); > > if (!valid_handle) { > > WARN(1, "%s: invalid handle passed to share.\n", __func__); > @@ > > -1117,7 +1147,6 @@ struct dma_buf *ion_share_dma_buf(struct ion_client > *client, > > } > > buffer = handle->buffer; > > ion_buffer_get(buffer); > > - mutex_unlock(&client->lock); > > > > exp_info.ops = &dma_buf_ops; > > exp_info.size = buffer->size; > > @@ -1132,14 +1161,26 @@ struct dma_buf *ion_share_dma_buf(struct > > ion_client *client, > > > > return dmabuf; > > } > > + > > +struct dma_buf *ion_share_dma_buf(struct ion_client *client, > > + struct ion_handle *handle) > > +{ > > + struct dma_buf *dmabuf; > > + > > + mutex_lock(&client->lock); > > + dmabuf = ion_share_dma_buf_nolock(client, handle); > > + mutex_unlock(&client->lock); > > + return dmabuf; > > +} > > EXPORT_SYMBOL(ion_share_dma_buf); > > > > -int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle > > *handle) > > +static int ion_share_dma_buf_fd_nolock(struct ion_client *client, > > + struct ion_handle *handle) > > { > > struct dma_buf *dmabuf; > > int fd; > > > > - dmabuf = ion_share_dma_buf(client, handle); > > + dmabuf = ion_share_dma_buf_nolock(client, handle); > > if (IS_ERR(dmabuf)) > > return PTR_ERR(dmabuf); > > > > @@ -1149,6 +1190,17 @@ int ion_share_dma_buf_fd(struct ion_client > > *client, struct ion_handle *handle) > > > > return fd; > > } > > + > > +int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle > > +*handle) { > > + int fd; > > + > > + mutex_lock(&client->lock); > > + fd = ion_share_dma_buf_fd_nolock(client, handle); > > + mutex_unlock(&client->lock); > > + > > + return fd; > > +} > > EXPORT_SYMBOL(ion_share_dma_buf_fd); > > > > struct ion_handle *ion_import_dma_buf(struct ion_client *client, int > > fd) @@ -1281,11 +1333,16 @@ static long ion_ioctl(struct file *filp, > unsigned int cmd, unsigned long arg) > > { > > struct ion_handle *handle; > > > > - handle = ion_handle_get_by_id(client, data.handle.handle); > > - if (IS_ERR(handle)) > > + mutex_lock(&client->lock); > > + handle = ion_handle_get_by_id_nolock(client, > > + data.handle.handle); > > + if (IS_ERR(handle)) { > > + mutex_unlock(&client->lock); > > return PTR_ERR(handle); > > - ion_free(client, handle); > > - ion_handle_put(handle); > > + } > > + ion_free_nolock(client, handle); > > + ion_handle_put_nolock(handle); > > + mutex_unlock(&client->lock); > > break; > > } > > case ION_IOC_SHARE: > > @@ -1293,11 +1350,16 @@ static long ion_ioctl(struct file *filp, > unsigned int cmd, unsigned long arg) > > { > > struct ion_handle *handle; > > > > - handle = ion_handle_get_by_id(client, data.handle.handle); > > - if (IS_ERR(handle)) > > + mutex_lock(&client->lock); > > + handle = ion_handle_get_by_id_nolock(client, > > + data.handle.handle); > > + if (IS_ERR(handle)) { > > + mutex_unlock(&client->lock); > > return PTR_ERR(handle); > > - data.fd.fd = ion_share_dma_buf_fd(client, handle); > > - ion_handle_put(handle); > > + } > > + data.fd.fd = ion_share_dma_buf_fd_nolock(client, handle); > > + ion_handle_put_nolock(handle); > > + mutex_unlock(&client->lock); > > if (data.fd.fd < 0) > > ret = data.fd.fd; > > break; > > > > I don't think this is necessary. We had the race in ION_IOC_FREE because > the free operation didn't happen atomically. It was possible to have two > different threads destroying the handle at the same time. With > ION_IOC_MAP/ION_IOC_SHARE, ion_handle_get_by_id will get a reference so > assuming there are no other races, that should ensure the handle will not > be destroyed. > > Is there another race you can see in the code that I missed? > I was thinking about ion_client_destroy being called when ION_IOC_MAP/ION_IOC_SHARE is executing. But I don't think that is possible. So I agree that we don't need to protect ION_IOC_MAP/ION_IOC_SHARE and ion_client_destroy with the mutex. Thanks, Euntaik