From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [PATCH 07/10] locks: define a lm_setup handler for leases Date: Sun, 24 Aug 2014 21:19:53 -0400 Message-ID: <20140824211953.1dfd7f77@synchrony.poochiereds.net> References: <1408804878-1331-1-git-send-email-jlayton@primarydata.com> <1408804878-1331-8-git-send-email-jlayton@primarydata.com> <20140824155858.GF15908@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, bfields@fieldses.org, cluster-devel@redhat.com, linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org To: Christoph Hellwig Return-path: In-Reply-To: <20140824155858.GF15908@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-cifs.vger.kernel.org On Sun, 24 Aug 2014 08:58:58 -0700 Christoph Hellwig wrote: > I like this change a lot! But one caveat: > > > + /* > > + * Despite the fact that it's an int return function, __f_setown never > > + * returns an error. Just ignore any error return here, but spew a > > + * WARN_ON_ONCE in case this ever changes. > > + */ > > + WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0)); > > I don't think this is a good idea. The errors in __f_setown come from > the security modules, and they could change easily. If you can convince > the LSM people to change their file_set_fowner routine to return void > we could change __f_setown to return void as well and be done with it, > but without that this looks too dangerous. > Understood. I figured that this might not be acceptable. I can make this propagate the error back up, but cleaning up the mess may not be easy. I'll see what I can do. Note that the error handling in the existing code looks wrong to me too. The lease gets added to the list (or updated), the fasync handler gets set up. Then, if __f_setown returns an error, the code just returns that error without unwinding anything. -- Jeff Layton From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Date: Sun, 24 Aug 2014 21:19:53 -0400 Subject: [Cluster-devel] [PATCH 07/10] locks: define a lm_setup handler for leases In-Reply-To: <20140824155858.GF15908@infradead.org> References: <1408804878-1331-1-git-send-email-jlayton@primarydata.com> <1408804878-1331-8-git-send-email-jlayton@primarydata.com> <20140824155858.GF15908@infradead.org> Message-ID: <20140824211953.1dfd7f77@synchrony.poochiereds.net> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Sun, 24 Aug 2014 08:58:58 -0700 Christoph Hellwig wrote: > I like this change a lot! But one caveat: > > > + /* > > + * Despite the fact that it's an int return function, __f_setown never > > + * returns an error. Just ignore any error return here, but spew a > > + * WARN_ON_ONCE in case this ever changes. > > + */ > > + WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0)); > > I don't think this is a good idea. The errors in __f_setown come from > the security modules, and they could change easily. If you can convince > the LSM people to change their file_set_fowner routine to return void > we could change __f_setown to return void as well and be done with it, > but without that this looks too dangerous. > Understood. I figured that this might not be acceptable. I can make this propagate the error back up, but cleaning up the mess may not be easy. I'll see what I can do. Note that the error handling in the existing code looks wrong to me too. The lease gets added to the list (or updated), the fasync handler gets set up. Then, if __f_setown returns an error, the code just returns that error without unwinding anything. -- Jeff Layton