smbd panic at find_oplock_types().

[Jeremy Allison]

On Fri, Sep 05, 2014 at 04:21:11PM -0700, Hemanth Thummala wrote:
> if (req == NULL) {
> oplock_request |= INTERNAL_OPEN_ONLY;
> }
> ...
> 
> But in open_file_ntcreate(), we are unconditionally clearing this
> INTERNAL_OPEN_ONLY flag.
> 
> ...
> /* Ensure no SAMBA_PRIVATE bits can be set. */
> fsp->oplock_type = (oplock_request & ~SAMBA_PRIVATE_OPLOCK_MASK);

Yes, we don't want to set any Samba private
bits in the field that gets stored inside
the share_mode_entry.

INTERNAL_OPEN_ONLY is a synonym for req==NULL,
and we probably can now just remove it as a
flag ever passed into SMB_VFS_CREATE_FILE(),
at least in master. It represents an open that
is being done on behalf of the smbd to achieve
another task, and not at the direct request of
the client (which is why req == NULL).

Inside  validate_my_share_entries() we check for
INTERNAL_OPEN_ONLY using the test:

        if (share_entry->share_file_id == 0) {
                /* INTERNAL_OPEN_ONLY */
                return;
        }

> Due to this, we never treated this shared lock entry as internal.
> 
> In find_oplock_types() we have the checks..
> 
> ...
> if ((oplock_request & INTERNAL_OPEN_ONLY) ||
> is_stat_open(fsp->access_mask)) {
> return;
> }
> ....
> if (lck->share_modes[i].op_type == NO_OPLOCK &&
> is_stat_open(lck->share_modes[i].access_mask)) {
> /* We ignore stat opens in the table - they
>    always have NO_OPLOCK and never get or
>    cause breaks. JRA. */
> continue;
> }
> ...
> 
> This particular shared lock is skipping all these checks. It was neither
> having INTERNAL_OPEN_FLAG nor a stat open(since access mask is zero).
> 
> I want to know..
> a) If its intentional to clear the SAMBA_PRIVATE_OPLOCK_MASK before adding
> the lock to share mode list.

Yes I think so. When we're looking in the share
mode table we expect any Samba-private bits not
to be there.

INTERNAL_OPEN_ONLY is should only be checked
as a request at open time, not stored in the db.

That was the intent originally. Obviously the
logic has gotten a little messed up :-).

> b) Why are we not using access mask as
> (SYNCHRONIZE_ACCESS|FILE_READ_ATTRIBUTES|
> FILE_WRITE_ATTRIBUTES) which will mark the open as stat_open and will allow
> to skip this share lock check. Thereby avoids panic.
> 
> Will file a bug for the same and put more details.

Thanks - as Volker said that's good enough
for us to add an smbtorture test so ensure
we don't regress when we fix this.

I'll take a look on Monday if Volker doesn't
get to it first :-).

Cheers,

	Jeremy.