From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: [PATCH] drm: fix i_mapping and f_mapping initialization in drm_open in error path Date: Sun, 31 Mar 2013 12:34:18 +0200 Message-ID: <20130331103418.GA18476@dhcp22.suse.cz> References: <20130326195601.GA5124@dhcp22.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f173.google.com (mail-ea0-f173.google.com [209.85.215.173]) by gabe.freedesktop.org (Postfix) with ESMTP id CF6A2E5C00 for ; Sun, 31 Mar 2013 03:34:22 -0700 (PDT) Received: by mail-ea0-f173.google.com with SMTP id k11so732994eaj.32 for ; Sun, 31 Mar 2013 03:34:22 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ilija Hadzic Cc: Thomas Hellstrom , Marco Munderloh , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Sat 30-03-13 18:26:53, Ilija Hadzic wrote: > This looks a bit like a hack and it doesn't look right, > conceptually. If the call fails, it should restore things as if > nothing has ever happened and overwriting old_mapping is not going to > do the trick. OK, I thought this is what the patch does as it falls back to &inode->i_data which is the default mapping for all inodes or it uses what used to be in device mapping. I am obviously not familiar with the drm code but it feels a bit strange that the device mapping can be different than inode's resp. file's one and even more confusing that inode and file are saved separately. > I think the right way to fix it would be to separately store the > original mapping for filp->f_mapping and inode->i_mapping and restore > it from their respective temporary variables if drm_open_helper or > drm_setup fail. Attached is a quick patch to show you [...] > @@ -137,6 +139,8 @@ int drm_open(struct inode *inode, struct file *filp) > if (!dev->open_count++) > need_setup = 1; > mutex_lock(&dev->struct_mutex); > + old_fmapping = filp->f_mapping; > + old_imapping = inode->i_mapping; How can file and inode mappings be different? > old_mapping = dev->dev_mapping; > if (old_mapping == NULL) > dev->dev_mapping = &inode->i_data; > @@ -159,8 +163,8 @@ int drm_open(struct inode *inode, struct file *filp) > > err_undo: > mutex_lock(&dev->struct_mutex); > - filp->f_mapping = old_mapping; > - inode->i_mapping = old_mapping; > + filp->f_mapping = old_fmapping; > + inode->i_mapping = old_imapping; > iput(container_of(dev->dev_mapping, struct inode, i_data)); > dev->dev_mapping = old_mapping; > mutex_unlock(&dev->struct_mutex); -- 1.8.1.5 -- Michal Hocko SUSE Labs