February 2013 - Wine-Devel - list.winehq.org

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by J. Bruce Fields

On Thu, Feb 07, 2013 at 06:32:38PM +0400, Pavel Shilovsky wrote: > 2013/2/7 J. Bruce Fields <bfields(a)fieldses.org>: > > On Thu, Feb 07, 2013 at 01:53:46PM +0400, Pavel Shilovsky wrote: > >> Nothing prevents it. If somebody grabbed a share mode lock on a file > >> before we call deny_lock_file, we simply close this file and return > >> -ETXTBSY. > > > > But leave the newly-created file there--ugh. > > > >> We can't grab it before atomic_open because we don't have an > >> inode there. > > > > If you can get the lock while still holding the directory i_mutex can't > > you prevent anyone else from looking up the new file until you've gotten > > the lock? > > > > Hm..., seems you are right, I missed this part: > mutex_lock > lookup_open -> atomic_open -> deny_lock_file > mutex_unlock > > that means that nobody can open and of course set flock on the newly > created file (because flock is done through file descriptor). So, it > should be fine to call flock after f_ops->atomic_open in atomic_open > function. Thanks. Whether that works may also depend on how the new dentry is set up? If it's hashed before you call flock then I suppose it's already visible to others. Not knowing that code as well as I should, I might test by introducing an artificial delay there and trying to reproduce the race. --b.

Feb. 7, 2013

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by Pavel Shilovsky

2013/2/7 J. Bruce Fields <bfields(a)fieldses.org>: > On Thu, Feb 07, 2013 at 01:53:46PM +0400, Pavel Shilovsky wrote: >> Nothing prevents it. If somebody grabbed a share mode lock on a file >> before we call deny_lock_file, we simply close this file and return >> -ETXTBSY. > > But leave the newly-created file there--ugh. > >> We can't grab it before atomic_open because we don't have an >> inode there. > > If you can get the lock while still holding the directory i_mutex can't > you prevent anyone else from looking up the new file until you've gotten > the lock? > Hm..., seems you are right, I missed this part: mutex_lock lookup_open -> atomic_open -> deny_lock_file mutex_unlock that means that nobody can open and of course set flock on the newly created file (because flock is done through file descriptor). So, it should be fine to call flock after f_ops->atomic_open in atomic_open function. Thanks. -- Best regards, Pavel Shilovsky.

Feb. 7, 2013

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by J. Bruce Fields

On Thu, Feb 07, 2013 at 01:53:46PM +0400, Pavel Shilovsky wrote: > 2013/2/5 J. Bruce Fields <bfields(a)fieldses.org>: > > On Tue, Feb 05, 2013 at 03:45:31PM +0400, Pavel Shilovsky wrote: > >> 2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > >> > On Thu, Jan 17, 2013 at 08:52:59PM +0400, Pavel Shilovsky wrote: > >> >> If O_DENYMAND flag is specified, O_DENYREAD/WRITE/MAND flags are > >> >> translated to flock's flags: > >> >> > >> >> !O_DENYREAD -> LOCK_READ > >> >> !O_DENYWRITE -> LOCK_WRITE > >> >> O_DENYMAND -> LOCK_MAND > >> >> > >> >> and set through flock_lock_file on a file. > >> >> > >> >> This change only affects opens that use O_DENYMAND flag - all other > >> >> native Linux opens don't care about these flags. It allow us to > >> >> enable this feature for applications that need it (e.g. NFS and > >> >> Samba servers that export the same directory for Windows clients, > >> >> or Wine applications that access the same files simultaneously). > >> > > >> > The use of an is_conflict callback seems unnecessarily convoluted. > >> > > >> > If we need two different behaviors, let's just use another flag (or an > >> > extra boolean argument if we need to, or something). > >> > >> Ok, we can pass "bool is_mand" to flock_lock_file that will pass it > >> further to flock_locks_conflict. > >> > >> > > >> > The only caller for this new deny_lock_file is in the nfs code--I'm a > >> > little unclear why that is. > >> > >> deny_lock_file is called not only in the nfs code but also in 2 places > >> of fs/namei.c -- that enable this logic for VFS. > > > > Oops, apologies, I overlooked those somehow. > > > > What prevents somebody else from grabbing a lock on a newly-created file > > before we grab our own lock? > > > > I couldn't tell on a quick look whether we hold some lock that prevents > > that. > > Nothing prevents it. If somebody grabbed a share mode lock on a file > before we call deny_lock_file, we simply close this file and return > -ETXTBSY. But leave the newly-created file there--ugh. > We can't grab it before atomic_open because we don't have an > inode there. If you can get the lock while still holding the directory i_mutex can't you prevent anyone else from looking up the new file until you've gotten the lock? --b. > Anyway, we can't make it atomic for VFS without big code > changes, but for CIFS and NFS it is already atomic with the discussed > patch.

Feb. 7, 2013

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by Pavel Shilovsky

2013/2/5 J. Bruce Fields <bfields(a)fieldses.org>: > On Tue, Feb 05, 2013 at 03:45:31PM +0400, Pavel Shilovsky wrote: >> 2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: >> > On Thu, Jan 17, 2013 at 08:52:59PM +0400, Pavel Shilovsky wrote: >> >> If O_DENYMAND flag is specified, O_DENYREAD/WRITE/MAND flags are >> >> translated to flock's flags: >> >> >> >> !O_DENYREAD -> LOCK_READ >> >> !O_DENYWRITE -> LOCK_WRITE >> >> O_DENYMAND -> LOCK_MAND >> >> >> >> and set through flock_lock_file on a file. >> >> >> >> This change only affects opens that use O_DENYMAND flag - all other >> >> native Linux opens don't care about these flags. It allow us to >> >> enable this feature for applications that need it (e.g. NFS and >> >> Samba servers that export the same directory for Windows clients, >> >> or Wine applications that access the same files simultaneously). >> > >> > The use of an is_conflict callback seems unnecessarily convoluted. >> > >> > If we need two different behaviors, let's just use another flag (or an >> > extra boolean argument if we need to, or something). >> >> Ok, we can pass "bool is_mand" to flock_lock_file that will pass it >> further to flock_locks_conflict. >> >> > >> > The only caller for this new deny_lock_file is in the nfs code--I'm a >> > little unclear why that is. >> >> deny_lock_file is called not only in the nfs code but also in 2 places >> of fs/namei.c -- that enable this logic for VFS. > > Oops, apologies, I overlooked those somehow. > > What prevents somebody else from grabbing a lock on a newly-created file > before we grab our own lock? > > I couldn't tell on a quick look whether we hold some lock that prevents > that. Nothing prevents it. If somebody grabbed a share mode lock on a file before we call deny_lock_file, we simply close this file and return -ETXTBSY. We can't grab it before atomic_open because we don't have an inode there. Anyway, we can't make it atomic for VFS without big code changes, but for CIFS and NFS it is already atomic with the discussed patch. -- Best regards, Pavel Shilovsky.

Feb. 7, 2013

Re: [PATCH v2 0/8] Add O_DENY* support for VFS and CIFS/NFS

by Pavel Shilovsky

2013/2/5 J. Bruce Fields <bfields(a)fieldses.org>: > On Tue, Feb 05, 2013 at 03:33:21PM +0400, Pavel Shilovsky wrote: >> 2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: >> > On Thu, Jan 17, 2013 at 08:52:09PM +0400, Pavel Shilovsky wrote: >> >> This patchset adds support of O_DENY* flags for Linux fs layer. These flags can be used by any application that needs share reservations to organize a file access. VFS already has some sort of this capability - now it's done through flock/LOCK_MAND mechanis, but that approach is non-atomic. This patchset build new capabilities on top of the existing one but doesn't bring any changes into the flock call semantic. >> >> >> >> These flags can be used by NFS (built-in-kernel) and CIFS (Samba) servers and Wine applications through VFS (for local filesystems) or CIFS/NFS modules. This will help when e.g. Samba and NFS server share the same directory for Windows and Linux users or Wine applications use Samba/NFS share to access the same data from different clients. >> >> >> >> According to the previous discussions the most problematic question is how to prevent situations like DoS attacks where e.g /lib/liba.so file can be open with DENYREAD, or smth like this. That's why one extra flag O_DENYMAND is added. It indicates to underlying layer that an application want to use O_DENY* flags semantic. It allows us not affect native Linux applications (that don't use O_DENYMAND flag) - so, these flags (and the semantic of open syscall that they bring) are used only for those applications that really want it proccessed that way. >> > >> > Maybe that's good enough. A mount flag might be simpler and give >> > consistent enforcement for all users. >> > >> >> >> >> So, we have four new flags: >> >> O_DENYREAD - to prevent other opens with read access, >> >> O_DENYWRITE - to prevent other opens with write access, >> >> O_DENYDELETE - to prevent delete operations (this flag is not implemented in VFS and NFS part and only suitable for CIFS module), >> >> O_DENYMAND - to switch on/off three flags above. >> > >> > It would be useful to have some really careful documentation of how >> > these are meant to work. Maybe try updating the open man page? >> >> Yes, that's a good idea. Do you mean smth like this? >> >> O_DENYMAND - used to inforce a mandatory share reservation scheme of >> the file access. If this flag is passed, >> the open fails with -ETXTBSY in following cases: >> >> 1) if O_DENYREAD flag is specified and there is another open with >> O_DENYMAND flag and READ access to the file; >> 2) if O_DENYWRITE flag is specified and there is another open with >> O_DENYMAND flag and WRITE access to the file; >> 3) if READ access is requested and there is another open with >> O_DENYMAND and O_DENYREAD flags; >> 4) if WRITE access is requested and there is another open with >> O_DENYMAND and O_DENYWRITE flags; >> >> Also, if O_DENYDELETE flag is specified and the open succeded, any >> further unlink operation will fail with -ETXTBSY untill this open is >> closed. Now this flag is processed by CIFS filesystems only. > > Do you need to document interactions with flock() as well? Yes, this needs to be documented, thanks. I am going to propose ver2 of this patchset (with regards to your suggestions) - will add this part to open man page patch too. -- Best regards, Pavel Shilovsky.

Feb. 7, 2013

Re: iphlpapi: Let C look like C.

by Michael Stefaniuc

Thanks Juan, On 02/07/2013 12:54 AM, Juan Lang wrote: > Hi Michael, this isn't actually a problem with your patch, just > something I spotted: > > On Wed, Feb 6, 2013 at 3:15 PM, Michael Stefaniuc <mstefani(a)redhat.com > <mailto:mstefani(a)redhat.com>> wrote: > > On 02/06/2013 11:16 PM, Austin English wrote: > > On Feb 6, 2013 11:13 PM, "Michael Stefaniuc" <mstefani(a)redhat.de > <mailto:mstefani(a)redhat.de> > > <mailto:mstefani(a)redhat.de <mailto:mstefani(a)redhat.de>>> wrote: > >> > >> --- > >> dlls/iphlpapi/ipstats.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c > >> index f8191ba..966d24e 100644 > >> --- a/dlls/iphlpapi/ipstats.c > >> +++ b/dlls/iphlpapi/ipstats.c > >> @@ -693,7 +693,7 @@ DWORD WINAPI GetIcmpStatisticsEx(PMIB_ICMP_EX > > stats, DWORD family) > >> } > >> > >> ret = GetIcmpStatistics(&ipv4stats); > >> - if SUCCEEDED(ret) > >> + if (SUCCEEDED(ret)) > > > This code is incorrect. All paths that set ret yield a non-negative > value for ret, so SUCCEEDED(ret) is always 1. Using SUCCEEDED/FAILED on > something other than an HRESULT is asking for trouble. The correct thing > to do is if (!ret). can you please submit the patch so you get the commit glory? thanks bye michael

Feb. 7, 2013

Re: iphlpapi: Let C look like C.

by Juan Lang

Hi Michael, this isn't actually a problem with your patch, just something I spotted: On Wed, Feb 6, 2013 at 3:15 PM, Michael Stefaniuc <mstefani(a)redhat.com>wrote: > On 02/06/2013 11:16 PM, Austin English wrote: > > On Feb 6, 2013 11:13 PM, "Michael Stefaniuc" <mstefani(a)redhat.de > > <mailto:mstefani(a)redhat.de>> wrote: > >> > >> --- > >> dlls/iphlpapi/ipstats.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c > >> index f8191ba..966d24e 100644 > >> --- a/dlls/iphlpapi/ipstats.c > >> +++ b/dlls/iphlpapi/ipstats.c > >> @@ -693,7 +693,7 @@ DWORD WINAPI GetIcmpStatisticsEx(PMIB_ICMP_EX > > stats, DWORD family) > >> } > >> > >> ret = GetIcmpStatistics(&ipv4stats); > >> - if SUCCEEDED(ret) > >> + if (SUCCEEDED(ret)) > This code is incorrect. All paths that set ret yield a non-negative value for ret, so SUCCEEDED(ret) is always 1. Using SUCCEEDED/FAILED on something other than an HRESULT is asking for trouble. The correct thing to do is if (!ret). --Juan

Feb. 6, 2013

Re: iphlpapi: Let C look like C.

by Michael Stefaniuc

On 02/06/2013 11:16 PM, Austin English wrote: > On Feb 6, 2013 11:13 PM, "Michael Stefaniuc" <mstefani(a)redhat.de > <mailto:mstefani(a)redhat.de>> wrote: >> >> --- >> dlls/iphlpapi/ipstats.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c >> index f8191ba..966d24e 100644 >> --- a/dlls/iphlpapi/ipstats.c >> +++ b/dlls/iphlpapi/ipstats.c >> @@ -693,7 +693,7 @@ DWORD WINAPI GetIcmpStatisticsEx(PMIB_ICMP_EX > stats, DWORD family) >> } >> >> ret = GetIcmpStatistics(&ipv4stats); >> - if SUCCEEDED(ret) >> + if (SUCCEEDED(ret)) >> { >> stats->icmpInStats.dwMsgs = ipv4stats.stats.icmpInStats.dwMsgs; >> stats->icmpInStats.dwErrors = > ipv4stats.stats.icmpInStats.dwErrors; >> -- >> 1.7.6.5 > > Minor nitpick, "Make C look like C" would be a more proper title, imho. Yeah, but then I would describe what I'm doing and that is a big NO NO as it is obvious from the patch. The intend is to let the Wine C code feel like a first class C citizen ;-P bye michael

Feb. 6, 2013

Re: iphlpapi: Let C look like C.

by Austin English

On Feb 6, 2013 11:13 PM, "Michael Stefaniuc" <mstefani(a)redhat.de> wrote: > > --- > dlls/iphlpapi/ipstats.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c > index f8191ba..966d24e 100644 > --- a/dlls/iphlpapi/ipstats.c > +++ b/dlls/iphlpapi/ipstats.c > @@ -693,7 +693,7 @@ DWORD WINAPI GetIcmpStatisticsEx(PMIB_ICMP_EX stats, DWORD family) > } > > ret = GetIcmpStatistics(&ipv4stats); > - if SUCCEEDED(ret) > + if (SUCCEEDED(ret)) > { > stats->icmpInStats.dwMsgs = ipv4stats.stats.icmpInStats.dwMsgs; > stats->icmpInStats.dwErrors = ipv4stats.stats.icmpInStats.dwErrors; > -- > 1.7.6.5 Minor nitpick, "Make C look like C" would be a more proper title, imho.

Feb. 6, 2013

Re: comctl32/tests: drop broken test

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24289 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

Feb. 6, 2013

Re: [1/4] windowscodecs: Implement IWICColorContext::InitializeFromFilename.

by Hans Leidekker

On Wed, 2013-02-06 at 18:10 +0100, Ludger Sprenker wrote: > On Wed, 06 Feb 2013 13:51:32 +0100, Hans Leidekker wrote: > > > + handle = CreateFileW(filename, GENERIC_READ, 0, NULL, > > > OPEN_EXISTING, 0, NULL); > > > + if (handle == INVALID_HANDLE_VALUE) return > > > HRESULT_FROM_WIN32(GetLastError()); > > I think, you are hiding a fixme for the default color profiles shipped with windows. > > (like "c:\windows\system32\spool\drivers\color\sRGB Color Space Profile.icm") I don't think this is the right place for such a fixme, it could just as well be loading a profile that comes bundled with the app. Also note that any attempt to use the color context will cause a fixme to be printed.

Feb. 6, 2013

Re: [4/4] windowscodecs: Implement IWICComponentFactory::CreateDecoderFromFileHandle.

by Hans Leidekker

On Wed, 2013-02-06 at 10:25 -0600, Vincent Povirk wrote: > > + hr = map_file(file, &map, &mem, &size); > > + if (FAILED(hr)) return hr; > > + > ... > > + > > + hr = IWICStreamImpl_InitializeFromMemory(stream, mem, size.u.LowPart); > > + if (FAILED(hr)) goto error; > > This makes me sad. > > Is this really simpler than implementing Write and Seek for a file handle? I'd say it's a comparable amount of code. I don't have a strong preference for either option.

Feb. 6, 2013

kernel32: Use the monotonic counter in GetTickCount64.

by Joerg-Cyril.Hoehle＠t-systems.com

Hi, I believe we should use this opportunity and review the usage of timers in Wine. E.g. I have a patch to have CreateTimerqueue use the monotonic timer too that I'll submit after some more testing. Mmdevapi's audio currently depends on the TimerQueue which IMHO erroneously depends on gettimeofday() instead of clocks not affected by DST and NTP. As a result, strange things should happen to Wine audio each time the clock is adjusted... What do you think? BTW, MSDN is wonderfully inconsistent - witness "system time": "System time is the current date and time of day. [...] The system bases system time on coordinated universal time (UTC)." http://msdn.microsoft.com/en-us/library/windows/desktop/ms724961%28v=vs.85%… "The system time is the time, in milliseconds, since the Microsoft Windows operating system was started." http://msdn.microsoft.com/en-us/library/windows/desktop/dd743637%28v=vs.85%… The latter is within the context of windows multimedia. I believe different teams at MS wrote documents without cross-checking their use of vocabulary. As a result, expressions don't mean the same things. Regards, Jörg Höhle

Feb. 6, 2013

Re: [1/4] windowscodecs: Implement IWICColorContext::InitializeFromFilename.

by Ludger Sprenker

On Wed, 06 Feb 2013 13:51:32 +0100, Hans Leidekker wrote: > + handle = CreateFileW(filename, GENERIC_READ, 0, NULL, > OPEN_EXISTING, 0, NULL); > + if (handle == INVALID_HANDLE_VALUE) return > HRESULT_FROM_WIN32(GetLastError()); Hello Hans, I think, you are hiding a fixme for the default color profiles shipped with windows. (like "c:\windows\system32\spool\drivers\color\sRGB Color Space Profile.icm") I don't know the correct way to support these files. Maybe wine could search in "/usr/share/color/icc" for compatible profiles on runtime or take these files from other open source projects and include them in the wine installation. Regards, Ludger

Feb. 6, 2013

Re: [4/4] windowscodecs: Implement IWICComponentFactory::CreateDecoderFromFileHandle.

by Vincent Povirk

> + hr = map_file(file, &map, &mem, &size); > + if (FAILED(hr)) return hr; > + ... > + > + hr = IWICStreamImpl_InitializeFromMemory(stream, mem, size.u.LowPart); > + if (FAILED(hr)) goto error; This makes me sad. Is this really simpler than implementing Write and Seek for a file handle?

Feb. 6, 2013

Re: kernel32/tests : added tests for copyfileex try7

by Alexandre Julliard

Patrick Rudolph <siro(a)das-labor.org> writes: > + if( !GetTempPathA( MAX_PATH,tmppath ) ) > + { > + win_skip("GetTempPathA failed\n"); > + return; > + } > + > + /* create a dummy file for copy tests */ > + if( !GetTempFileNameA( tmppath,"IN",0,source_name ) ) > + { > + win_skip("GetTempFileNameA failed\n"); > + return; > + } > + > + /* create a dummy file for copy tests */ > + if( !GetTempFileNameA( tmppath,"OUT",0,dest_name ) ) > + { > + win_skip("GetTempFileNameA failed\n"); > + return; > + } > + > + source_handle = CreateFileA( source_name, GENERIC_WRITE, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0 ); > + if( !source_handle ) > + { > + win_skip("CreateFileA failed (%d)\n", GetLastError()); > + return; > + } > + > +#define COPYFILEEXFILESIZE 10000000 > + > + /* set filesize to COPYFILEEXFILESIZE */ > + TotalFileSize.QuadPart = COPYFILEEXFILESIZE; > + > + if( !pSetFilePointerEx(source_handle,TotalFileSize,NULL,FILE_BEGIN) ) > + { > + win_skip("SetFilePointerEx failed (%d)\n", GetLastError()); > + return; > + } > + > + if( !pSetEndOfFile(source_handle) ) > + { > + win_skip("SetEndOfFile failed (%d)\n", GetLastError()); > + return; > + } All these calls are not supposed to fail on Windows either. win_skip is not appropriate. -- Alexandre Julliard julliard(a)winehq.org

Feb. 6, 2013

Re: [PATCH] Support a protection scheme which reset the GS selector

by Alexandre Julliard

Alessandro Pignotti <a.pignotti(a)sssup.it> writes: > Hi everyone, > > I've found two different games: > > -) Of orcs and men > -) The testament of sherlock holmes > > which are using a protection scheme which reset the GS segment selector, > possibly to confuse virtual machines. Since on linux the GS selector is > used internally by glibc the game would crash as soon as a native glibc > method is called by wine. The proposed patch tries to workaround this > problem by restoring the GS to a known working value in wine's SEGFAULS > handler and try the instruction again. This should be done similarly to the other checks in raise_segv_exception. -- Alexandre Julliard julliard(a)winehq.org

Feb. 6, 2013

Re: [2/5] windowscodecs: Basic tests for tiff encoder options (try2)

by Vincent Povirk

> @Vincent: I thought about adding the properties one at a time but it seemed > a bit like overkill for the tiff encoder, which has only two options. But I > will defenitly do this for the jpeg encoder, which has much more options. Well, actually, on further consideration I can understand why you did it this way, since adding a property is very easy and doesn't really require implementing that property. Though it'd be nice to get some notification when a property is used that we don't support.

Feb. 5, 2013

Re: [2/5] windowscodecs: Basic tests for tiff encoder options

by Vincent Povirk

+ { wszTiffCompressionMethod, VT_UI1, VT_UI1, {.iVal=WICTiffCompressionDontCare} }, This .field=value initialization syntax is not acceptable for Wine (not in C89). I don't know another way to initialize unions. + if (clsid_encoder == &CLSID_WICTiffEncoder) + todo_wine test_specific_encoder_properties(options, testdata_tiff_props, all_props, cProperties2); It might be better to mark the individual properties with a todo, so they can be added one at a time.

Feb. 5, 2013

Re: [PATCH v2 0/8] Add O_DENY* support for VFS and CIFS/NFS

by J. Bruce Fields

On Tue, Feb 05, 2013 at 03:33:21PM +0400, Pavel Shilovsky wrote: > 2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > > On Thu, Jan 17, 2013 at 08:52:09PM +0400, Pavel Shilovsky wrote: > >> This patchset adds support of O_DENY* flags for Linux fs layer. These flags can be used by any application that needs share reservations to organize a file access. VFS already has some sort of this capability - now it's done through flock/LOCK_MAND mechanis, but that approach is non-atomic. This patchset build new capabilities on top of the existing one but doesn't bring any changes into the flock call semantic. > >> > >> These flags can be used by NFS (built-in-kernel) and CIFS (Samba) servers and Wine applications through VFS (for local filesystems) or CIFS/NFS modules. This will help when e.g. Samba and NFS server share the same directory for Windows and Linux users or Wine applications use Samba/NFS share to access the same data from different clients. > >> > >> According to the previous discussions the most problematic question is how to prevent situations like DoS attacks where e.g /lib/liba.so file can be open with DENYREAD, or smth like this. That's why one extra flag O_DENYMAND is added. It indicates to underlying layer that an application want to use O_DENY* flags semantic. It allows us not affect native Linux applications (that don't use O_DENYMAND flag) - so, these flags (and the semantic of open syscall that they bring) are used only for those applications that really want it proccessed that way. > > > > Maybe that's good enough. A mount flag might be simpler and give > > consistent enforcement for all users. > > > >> > >> So, we have four new flags: > >> O_DENYREAD - to prevent other opens with read access, > >> O_DENYWRITE - to prevent other opens with write access, > >> O_DENYDELETE - to prevent delete operations (this flag is not implemented in VFS and NFS part and only suitable for CIFS module), > >> O_DENYMAND - to switch on/off three flags above. > > > > It would be useful to have some really careful documentation of how > > these are meant to work. Maybe try updating the open man page? > > Yes, that's a good idea. Do you mean smth like this? > > O_DENYMAND - used to inforce a mandatory share reservation scheme of > the file access. If this flag is passed, > the open fails with -ETXTBSY in following cases: > > 1) if O_DENYREAD flag is specified and there is another open with > O_DENYMAND flag and READ access to the file; > 2) if O_DENYWRITE flag is specified and there is another open with > O_DENYMAND flag and WRITE access to the file; > 3) if READ access is requested and there is another open with > O_DENYMAND and O_DENYREAD flags; > 4) if WRITE access is requested and there is another open with > O_DENYMAND and O_DENYWRITE flags; > > Also, if O_DENYDELETE flag is specified and the open succeded, any > further unlink operation will fail with -ETXTBSY untill this open is > closed. Now this flag is processed by CIFS filesystems only. Do you need to document interactions with flock() as well? --b.

Feb. 5, 2013

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by J. Bruce Fields

On Tue, Feb 05, 2013 at 03:45:31PM +0400, Pavel Shilovsky wrote: > 2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > > On Thu, Jan 17, 2013 at 08:52:59PM +0400, Pavel Shilovsky wrote: > >> If O_DENYMAND flag is specified, O_DENYREAD/WRITE/MAND flags are > >> translated to flock's flags: > >> > >> !O_DENYREAD -> LOCK_READ > >> !O_DENYWRITE -> LOCK_WRITE > >> O_DENYMAND -> LOCK_MAND > >> > >> and set through flock_lock_file on a file. > >> > >> This change only affects opens that use O_DENYMAND flag - all other > >> native Linux opens don't care about these flags. It allow us to > >> enable this feature for applications that need it (e.g. NFS and > >> Samba servers that export the same directory for Windows clients, > >> or Wine applications that access the same files simultaneously). > > > > The use of an is_conflict callback seems unnecessarily convoluted. > > > > If we need two different behaviors, let's just use another flag (or an > > extra boolean argument if we need to, or something). > > Ok, we can pass "bool is_mand" to flock_lock_file that will pass it > further to flock_locks_conflict. > > > > > The only caller for this new deny_lock_file is in the nfs code--I'm a > > little unclear why that is. > > deny_lock_file is called not only in the nfs code but also in 2 places > of fs/namei.c -- that enable this logic for VFS. Oops, apologies, I overlooked those somehow. What prevents somebody else from grabbing a lock on a newly-created file before we grab our own lock? I couldn't tell on a quick look whether we hold some lock that prevents that. --b.

Feb. 5, 2013

Re: [PATCH v2 4/8] CIFS: Add O_DENY* open flags support

by Pavel Shilovsky

2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > On Thu, Jan 17, 2013 at 08:53:00PM +0400, Pavel Shilovsky wrote: >> Make CIFSSMBOpen take share_flags as a parm that allows us >> to pass new O_DENY* flags to the server. >> >> Signed-off-by: Pavel Shilovsky <piastry(a)etersoft.ru> >> --- >> fs/cifs/cifsacl.c | 10 ++++++---- >> fs/cifs/cifsglob.h | 12 +++++++++++- >> fs/cifs/cifsproto.h | 9 +++++---- >> fs/cifs/cifssmb.c | 47 +++++++++++++++++++++++++---------------------- >> fs/cifs/dir.c | 13 ++++++++----- >> fs/cifs/file.c | 12 ++++++++---- >> fs/cifs/inode.c | 11 ++++++----- >> fs/cifs/link.c | 10 +++++----- >> fs/cifs/readdir.c | 2 +- >> fs/cifs/smb1ops.c | 15 ++++++++------- >> fs/cifs/smb2file.c | 10 +++++----- >> fs/cifs/smb2inode.c | 4 ++-- >> fs/cifs/smb2ops.c | 10 ++++++---- >> fs/cifs/smb2pdu.c | 6 +++--- >> fs/cifs/smb2proto.h | 14 ++++++++------ >> 15 files changed, 107 insertions(+), 78 deletions(-) >> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c >> index 0fb15bb..a42c77f 100644 >> --- a/fs/cifs/cifsacl.c >> +++ b/fs/cifs/cifsacl.c >> @@ -1184,8 +1184,9 @@ static struct cifs_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, >> create_options |= CREATE_OPEN_BACKUP_INTENT; >> >> rc = CIFSSMBOpen(xid, tcon, path, FILE_OPEN, READ_CONTROL, >> - create_options, &fid, &oplock, NULL, cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); >> + FILE_SHARE_ALL, create_options, &fid, &oplock, >> + NULL, cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); >> if (!rc) { >> rc = CIFSSMBGetCIFSACL(xid, tcon, fid, &pntsd, pacllen); >> CIFSSMBClose(xid, tcon, fid); >> @@ -1245,8 +1246,9 @@ int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen, >> access_flags = WRITE_DAC; >> >> rc = CIFSSMBOpen(xid, tcon, path, FILE_OPEN, access_flags, >> - create_options, &fid, &oplock, NULL, cifs_sb->local_nls, >> - cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); >> + FILE_SHARE_ALL, create_options, &fid, &oplock, >> + NULL, cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > > It would be easier to tell what you changed in the above two chunks if > you didn't change the whitespace at the same time. > > --b. Ok, thank - will fix it in the further version of the patch. -- Best regards, Pavel Shilovsky.

Feb. 5, 2013

Re: [PATCH v2 3/8] vfs: Add O_DENYREAD/WRITE flags support for open syscall

by Pavel Shilovsky

2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > On Thu, Jan 17, 2013 at 08:52:59PM +0400, Pavel Shilovsky wrote: >> If O_DENYMAND flag is specified, O_DENYREAD/WRITE/MAND flags are >> translated to flock's flags: >> >> !O_DENYREAD -> LOCK_READ >> !O_DENYWRITE -> LOCK_WRITE >> O_DENYMAND -> LOCK_MAND >> >> and set through flock_lock_file on a file. >> >> This change only affects opens that use O_DENYMAND flag - all other >> native Linux opens don't care about these flags. It allow us to >> enable this feature for applications that need it (e.g. NFS and >> Samba servers that export the same directory for Windows clients, >> or Wine applications that access the same files simultaneously). > > The use of an is_conflict callback seems unnecessarily convoluted. > > If we need two different behaviors, let's just use another flag (or an > extra boolean argument if we need to, or something). Ok, we can pass "bool is_mand" to flock_lock_file that will pass it further to flock_locks_conflict. > > The only caller for this new deny_lock_file is in the nfs code--I'm a > little unclear why that is. deny_lock_file is called not only in the nfs code but also in 2 places of fs/namei.c -- that enable this logic for VFS. -- Best regards, Pavel Shilovsky.

Feb. 5, 2013

Re: [PATCH v2 0/8] Add O_DENY* support for VFS and CIFS/NFS

by Pavel Shilovsky

2013/1/31 J. Bruce Fields <bfields(a)fieldses.org>: > On Thu, Jan 17, 2013 at 08:52:09PM +0400, Pavel Shilovsky wrote: >> This patchset adds support of O_DENY* flags for Linux fs layer. These flags can be used by any application that needs share reservations to organize a file access. VFS already has some sort of this capability - now it's done through flock/LOCK_MAND mechanis, but that approach is non-atomic. This patchset build new capabilities on top of the existing one but doesn't bring any changes into the flock call semantic. >> >> These flags can be used by NFS (built-in-kernel) and CIFS (Samba) servers and Wine applications through VFS (for local filesystems) or CIFS/NFS modules. This will help when e.g. Samba and NFS server share the same directory for Windows and Linux users or Wine applications use Samba/NFS share to access the same data from different clients. >> >> According to the previous discussions the most problematic question is how to prevent situations like DoS attacks where e.g /lib/liba.so file can be open with DENYREAD, or smth like this. That's why one extra flag O_DENYMAND is added. It indicates to underlying layer that an application want to use O_DENY* flags semantic. It allows us not affect native Linux applications (that don't use O_DENYMAND flag) - so, these flags (and the semantic of open syscall that they bring) are used only for those applications that really want it proccessed that way. > > Maybe that's good enough. A mount flag might be simpler and give > consistent enforcement for all users. > >> >> So, we have four new flags: >> O_DENYREAD - to prevent other opens with read access, >> O_DENYWRITE - to prevent other opens with write access, >> O_DENYDELETE - to prevent delete operations (this flag is not implemented in VFS and NFS part and only suitable for CIFS module), >> O_DENYMAND - to switch on/off three flags above. > > It would be useful to have some really careful documentation of how > these are meant to work. Maybe try updating the open man page? Yes, that's a good idea. Do you mean smth like this? O_DENYMAND - used to inforce a mandatory share reservation scheme of the file access. If this flag is passed, the open fails with -ETXTBSY in following cases: 1) if O_DENYREAD flag is specified and there is another open with O_DENYMAND flag and READ access to the file; 2) if O_DENYWRITE flag is specified and there is another open with O_DENYMAND flag and WRITE access to the file; 3) if READ access is requested and there is another open with O_DENYMAND and O_DENYREAD flags; 4) if WRITE access is requested and there is another open with O_DENYMAND and O_DENYWRITE flags; Also, if O_DENYDELETE flag is specified and the open succeded, any further unlink operation will fail with -ETXTBSY untill this open is closed. Now this flag is processed by CIFS filesystems only. -- Best regards, Pavel Shilovsky.

Feb. 5, 2013

Re: Compiling Wine on Mac OS X 10.8.2?

by Jeremiah Flerchinger

Chip, I'm compiling from wine-git again. I had to install the new command line tools for 4.6 and then reboot, since XCode 4.6 didn't seem to be enough. It seems an odd behavior that the patches appeared to force winebuild to use clang, when it had been using gcc before. Thanks and I'll try to see if can switch entirely to clang in my toolchain now. J On Mon, Feb 4, 2013 at 9:32 PM, Charles Davis <cdavis5x(a)gmail.com> wrote: > > On Feb 4, 2013, at 10:27 AM, Jeremiah Flerchinger wrote: > > Apple's GCC 4.2.1 version 5666.3 > > > > commdlg.vqed9R.s:259:2: error: invalid operand for instruction > lrecompobj.IYZvhS.st:w > 1037 : 2 : error: invalid operand for instruction > ^ > lretw > ^ > compobj.IYZvhS.s:1150:2: error: invalid operand for instruction > lretw > ^ > compobj.IYZvhS.s:1195:2: error: invalid operand for instruction > lretw > ^ > compobj.IYZvhS.s:1204:2: error: invalid operand for instruction > lretw > ^ > compobj.IYZvhS.s:1213:2: error: invalid operand for instruction > lretw > ^ > compobj.IYZvhS.s:1222:2: error: invalid operand for instruction > lretw > ^ > compobj.IYZvhS.s:1231:2: error: invalid operand for instruction > lretw > ^ > winebuild: /usr/bin/clang failed with status 1 > winegcc: ../../tools/winebuild/ > winebuild failed > make[1]: *** [commdlg.dll16.so] Error 2 > make: *** [dlls/commdlg.dll16] Error 2 > make: *** Waiting for unfinished jobs.... > ../../../tools/winegcc/winegcc -m32 -B../../../tools/winebuild > --sysroot=../../.. -s -Wb,-F,credui_test.exe credui.o testlist.o > -o credui_test-stripped.exe.so ../../../libs/port/libwine_port.a > -lcredui -L/Users/jeremiah/wine/wine-git/lib -L/opt/X11/lib -framework > CoreServices -lz -L/opt/X11/lib -lGL -lGLU > winebuild: /usr/bin/clang failed with status 1 > winegcc: ../../tools/winebuild/winebuild failed > make[1]: *** [compobj.dll16.so] Error 2 > make: *** [dlls/compobj.dll16] Error 2 > echo "credui_test.exe TESTRES \"credui_test-stripped.exe.so\"" | > ../../../tools/wrc/wrc --nostdinc --po-dir=../../../po -m32 -I. -I. > -I../../../include -I../../../include -DWINE_STRICT_PROTOTYPES > -DWINE_NO_NAMELESS_EXTENSION -DWIDL_C_INLINE_WRAPPERS -o > ../../../programs/winetest/credui_test.res > could not run 'make -j3' in /Users/jeremiah/wine/build/wine-git - exiting > > Yep, this is related to my patch alright. > > I'm willing to bet you're still running Xcode 4.5, too. Try upgrading to > 4.6. > > Chip > > > > On Sun, Feb 3, 2013 at 11:02 PM, Bill Broach <while1eq1.lists(a)gmail.com>wrote: > >> Which version of gcc are you compiling it with? >> >> Im able to compile no issue with: i686-apple-darwin11-gcc-4.2.1 (GCC) >> 4.2.1 (Apple Inc. build 5666) (dot 3) >> >> On Feb 3, 2013, at 3:58 PM, Charles Davis <cdavis5x(a)gmail.com> wrote: >> >> > >> > On Feb 3, 2013, at 10:50 AM, Saulius Krasuckas wrote: >> > >> >> * On Sun, 3 Feb 2013, Jeremiah Flerchinger wrote: >> >> >> >>> Is anyone compiling Wine from the git repository for OS X version >> >>> 10.8.2? >> >> >> >> Not me... >> >> >> >>> At first I thought I broke my toolchain and could no longer compile >> Wine >> >>> at all, but this is not the case. I can still compile old releases, >> but >> >>> can no longer compile from git. >> >> >> >> ... but I would try running a regression test (git bisect) in your >> case. >> > Why do I get the feeling that when you do, you'll end up at commit >> c14bdaf? >> > >> > How exactly does it break? >> > >> > Chip >> > >> >> >> >> S. >> >> >> >> >> > >> > >> > >> >> > >

Feb. 5, 2013