* [PATCH] eCryptfs: Fix directory open regression in linux-stable
@ 2016-06-29 3:39 Tyler Hicks
2016-07-05 21:14 ` Jeff Mahoney
0 siblings, 1 reply; 6+ messages in thread
From: Tyler Hicks @ 2016-06-29 3:39 UTC (permalink / raw)
To: stable; +Cc: ecryptfs
Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
introduces a regression in eCryptfs when mainline commit
6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
regression causes all attempts at opening directory files to fail with
EMEDIUMTYPE when the lower filesystem's file_operations for directory
files do not implement mmap.
This is a simple fix that allows the check for the lower file's mmap
implementation to be ignored if the lower file is a directory.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
Cc: <stable@vger.kernel.org> # 4.5-
---
fs/ecryptfs/kthread.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
index e818f5a..b9faeab 100644
--- a/fs/ecryptfs/kthread.c
+++ b/fs/ecryptfs/kthread.c
@@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
goto out;
}
have_file:
- if ((*lower_file)->f_op->mmap == NULL) {
+ if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
fput(*lower_file);
*lower_file = NULL;
rc = -EMEDIUMTYPE;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable
2016-06-29 3:39 [PATCH] eCryptfs: Fix directory open regression in linux-stable Tyler Hicks
@ 2016-07-05 21:14 ` Jeff Mahoney
2016-07-07 14:24 ` Henry Jensen
2016-07-07 18:02 ` Tyler Hicks
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Mahoney @ 2016-07-05 21:14 UTC (permalink / raw)
To: Tyler Hicks, stable; +Cc: ecryptfs
[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]
On 6/28/16 11:39 PM, Tyler Hicks wrote:
> Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
> introduces a regression in eCryptfs when mainline commit
> 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
> regression causes all attempts at opening directory files to fail with
> EMEDIUMTYPE when the lower filesystem's file_operations for directory
> files do not implement mmap.
>
> This is a simple fix that allows the check for the lower file's mmap
> implementation to be ignored if the lower file is a directory.
I have a different fix that I believe is more correct for this. I would
have posted it in response to the original fix if it were ever actually
posted for public discussion.
Denying open is the wrong place to fix this. It's too heavy a hammer
and, as we see here, a bit fragile.
The right fix is to deny the mmap call instead.
-Jeff
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
> Cc: <stable@vger.kernel.org> # 4.5-
> ---
> fs/ecryptfs/kthread.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
> index e818f5a..b9faeab 100644
> --- a/fs/ecryptfs/kthread.c
> +++ b/fs/ecryptfs/kthread.c
> @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
> goto out;
> }
> have_file:
> - if ((*lower_file)->f_op->mmap == NULL) {
> + if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
> fput(*lower_file);
> *lower_file = NULL;
> rc = -EMEDIUMTYPE;
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable
2016-07-05 21:14 ` Jeff Mahoney
@ 2016-07-07 14:24 ` Henry Jensen
2016-07-07 18:02 ` Tyler Hicks
1 sibling, 0 replies; 6+ messages in thread
From: Henry Jensen @ 2016-07-07 14:24 UTC (permalink / raw)
To: ecryptfs
Hello,
it seems, that none of the suggested patches will be included in
4.4.15? GKH doesn't mention one in his mail "4.4.15-stable review" on
the stable list.
Regards,
Henry
On Tue, 5 Jul 2016 17:14:20 -0400
Jeff Mahoney <jeffm@suse.com> wrote:
> On 6/28/16 11:39 PM, Tyler Hicks wrote:
> > Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
> > introduces a regression in eCryptfs when mainline commit
> > 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
> > regression causes all attempts at opening directory files to fail with
> > EMEDIUMTYPE when the lower filesystem's file_operations for directory
> > files do not implement mmap.
> >
> > This is a simple fix that allows the check for the lower file's mmap
> > implementation to be ignored if the lower file is a directory.
>
> I have a different fix that I believe is more correct for this. I would
> have posted it in response to the original fix if it were ever actually
> posted for public discussion.
>
> Denying open is the wrong place to fix this. It's too heavy a hammer
> and, as we see here, a bit fragile.
>
> The right fix is to deny the mmap call instead.
>
> -Jeff
>
> > Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> > Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
> > Cc: <stable@vger.kernel.org> # 4.5-
> > ---
> > fs/ecryptfs/kthread.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
> > index e818f5a..b9faeab 100644
> > --- a/fs/ecryptfs/kthread.c
> > +++ b/fs/ecryptfs/kthread.c
> > @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
> > goto out;
> > }
> > have_file:
> > - if ((*lower_file)->f_op->mmap == NULL) {
> > + if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
> > fput(*lower_file);
> > *lower_file = NULL;
> > rc = -EMEDIUMTYPE;
> >
>
>
> --
> Jeff Mahoney
> SUSE Labs
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable
2016-07-05 21:14 ` Jeff Mahoney
2016-07-07 14:24 ` Henry Jensen
@ 2016-07-07 18:02 ` Tyler Hicks
2016-07-07 18:19 ` Jeff Mahoney
1 sibling, 1 reply; 6+ messages in thread
From: Tyler Hicks @ 2016-07-07 18:02 UTC (permalink / raw)
To: Jeff Mahoney, stable; +Cc: ecryptfs
[-- Attachment #1.1: Type: text/plain, Size: 2160 bytes --]
On 07/05/2016 04:14 PM, Jeff Mahoney wrote:
> On 6/28/16 11:39 PM, Tyler Hicks wrote:
>> Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
>> introduces a regression in eCryptfs when mainline commit
>> 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
>> regression causes all attempts at opening directory files to fail with
>> EMEDIUMTYPE when the lower filesystem's file_operations for directory
>> files do not implement mmap.
>>
>> This is a simple fix that allows the check for the lower file's mmap
>> implementation to be ignored if the lower file is a directory.
>
> I have a different fix that I believe is more correct for this. I would
> have posted it in response to the original fix if it were ever actually
> posted for public discussion.
Hi Jeff - I'm glad that you are sending your fix out for review (not
sure if you noticed but I asked you to do so in a reply to your comment
on the project zero blog post).
> Denying open is the wrong place to fix this. It's too heavy a hammer
> and, as we see here, a bit fragile.
I agree but I think that denying open() has little to do with this
regression in linux-stable. I'd prefer that we leave linux-stable as-is
(after applying my minimal regression fix patch) and take your fix trunk.
Tyler
>
> The right fix is to deny the mmap call instead.
>
> -Jeff
>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
>> Cc: <stable@vger.kernel.org> # 4.5-
>> ---
>> fs/ecryptfs/kthread.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
>> index e818f5a..b9faeab 100644
>> --- a/fs/ecryptfs/kthread.c
>> +++ b/fs/ecryptfs/kthread.c
>> @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
>> goto out;
>> }
>> have_file:
>> - if ((*lower_file)->f_op->mmap == NULL) {
>> + if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
>> fput(*lower_file);
>> *lower_file = NULL;
>> rc = -EMEDIUMTYPE;
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable
2016-07-07 18:02 ` Tyler Hicks
@ 2016-07-07 18:19 ` Jeff Mahoney
2016-07-07 23:17 ` Tyler Hicks
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Mahoney @ 2016-07-07 18:19 UTC (permalink / raw)
To: Tyler Hicks, stable; +Cc: ecryptfs
[-- Attachment #1.1: Type: text/plain, Size: 2608 bytes --]
On 7/7/16 2:02 PM, Tyler Hicks wrote:
> On 07/05/2016 04:14 PM, Jeff Mahoney wrote:
>> On 6/28/16 11:39 PM, Tyler Hicks wrote:
>>> Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
>>> introduces a regression in eCryptfs when mainline commit
>>> 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
>>> regression causes all attempts at opening directory files to fail with
>>> EMEDIUMTYPE when the lower filesystem's file_operations for directory
>>> files do not implement mmap.
>>>
>>> This is a simple fix that allows the check for the lower file's mmap
>>> implementation to be ignored if the lower file is a directory.
>>
>> I have a different fix that I believe is more correct for this. I would
>> have posted it in response to the original fix if it were ever actually
>> posted for public discussion.
>
> Hi Jeff - I'm glad that you are sending your fix out for review (not
> sure if you noticed but I asked you to do so in a reply to your comment
> on the project zero blog post).
I didn't see that. Strange that it linked my profile pic but didn't
send a notification. :)
>> Denying open is the wrong place to fix this. It's too heavy a hammer
>> and, as we see here, a bit fragile.
>
> I agree but I think that denying open() has little to do with this
> regression in linux-stable. I'd prefer that we leave linux-stable as-is
> (after applying my minimal regression fix patch) and take your fix trunk.
It doesn't have anything to do with the regression -- but it can be
backported directly to stable without regressions if the original fix is
removed and stable fixes should match the upstream ones.
-Jeff
> Tyler
>
>>
>> The right fix is to deny the mmap call instead.
>>
>> -Jeff
>>
>>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>>> Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
>>> Cc: <stable@vger.kernel.org> # 4.5-
>>> ---
>>> fs/ecryptfs/kthread.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
>>> index e818f5a..b9faeab 100644
>>> --- a/fs/ecryptfs/kthread.c
>>> +++ b/fs/ecryptfs/kthread.c
>>> @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>> goto out;
>>> }
>>> have_file:
>>> - if ((*lower_file)->f_op->mmap == NULL) {
>>> + if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
>>> fput(*lower_file);
>>> *lower_file = NULL;
>>> rc = -EMEDIUMTYPE;
>>>
>>
>>
>
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable
2016-07-07 18:19 ` Jeff Mahoney
@ 2016-07-07 23:17 ` Tyler Hicks
0 siblings, 0 replies; 6+ messages in thread
From: Tyler Hicks @ 2016-07-07 23:17 UTC (permalink / raw)
To: Jeff Mahoney, stable; +Cc: ecryptfs
[-- Attachment #1.1: Type: text/plain, Size: 2952 bytes --]
On 07/07/2016 01:19 PM, Jeff Mahoney wrote:
> On 7/7/16 2:02 PM, Tyler Hicks wrote:
>> On 07/05/2016 04:14 PM, Jeff Mahoney wrote:
>>> On 6/28/16 11:39 PM, Tyler Hicks wrote:
>>>> Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
>>>> introduces a regression in eCryptfs when mainline commit
>>>> 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
>>>> regression causes all attempts at opening directory files to fail with
>>>> EMEDIUMTYPE when the lower filesystem's file_operations for directory
>>>> files do not implement mmap.
>>>>
>>>> This is a simple fix that allows the check for the lower file's mmap
>>>> implementation to be ignored if the lower file is a directory.
>>>
>>> I have a different fix that I believe is more correct for this. I would
>>> have posted it in response to the original fix if it were ever actually
>>> posted for public discussion.
>>
>> Hi Jeff - I'm glad that you are sending your fix out for review (not
>> sure if you noticed but I asked you to do so in a reply to your comment
>> on the project zero blog post).
>
> I didn't see that. Strange that it linked my profile pic but didn't
> send a notification. :)
>
>>> Denying open is the wrong place to fix this. It's too heavy a hammer
>>> and, as we see here, a bit fragile.
>>
>> I agree but I think that denying open() has little to do with this
>> regression in linux-stable. I'd prefer that we leave linux-stable as-is
>> (after applying my minimal regression fix patch) and take your fix trunk.
>
> It doesn't have anything to do with the regression -- but it can be
> backported directly to stable without regressions if the original fix is
> removed and stable fixes should match the upstream ones.
After reviewing and testing, I've changed my mind. I agree that your
patches are the better fix for trunk and stable. I do have some small
changes to make to the second patch. I'll reply directly to that patch
with those changes.
Tyler
>
> -Jeff
>
>> Tyler
>>
>>>
>>> The right fix is to deny the mmap call instead.
>>>
>>> -Jeff
>>>
>>>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>>>> Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
>>>> Cc: <stable@vger.kernel.org> # 4.5-
>>>> ---
>>>> fs/ecryptfs/kthread.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
>>>> index e818f5a..b9faeab 100644
>>>> --- a/fs/ecryptfs/kthread.c
>>>> +++ b/fs/ecryptfs/kthread.c
>>>> @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>>> goto out;
>>>> }
>>>> have_file:
>>>> - if ((*lower_file)->f_op->mmap == NULL) {
>>>> + if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
>>>> fput(*lower_file);
>>>> *lower_file = NULL;
>>>> rc = -EMEDIUMTYPE;
>>>>
>>>
>>>
>>
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-07 23:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-29 3:39 [PATCH] eCryptfs: Fix directory open regression in linux-stable Tyler Hicks
2016-07-05 21:14 ` Jeff Mahoney
2016-07-07 14:24 ` Henry Jensen
2016-07-07 18:02 ` Tyler Hicks
2016-07-07 18:19 ` Jeff Mahoney
2016-07-07 23:17 ` Tyler Hicks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).