Quota patches

[Jeremy Allison]

On Mon, Jul 30, 2018 at 09:23:25PM +0100, Noel Power wrote:
> Hi Jeremy
> 
> 
> On 27/07/18 19:05, Jeremy Allison wrote:
> > On Fri, Jul 27, 2018 at 01:51:10PM +0100, Noel Power via samba-technical wrote:
> [...]
> > OK, here is a version I'm OK with.
> >
> > To be honest, I'd love to see the code split up into
> > micro-patches with get/set done separately, but to
> > quote one of my favorite James Bond villains "It's
> > late, I'm tired, and there's so much left to do.." :-).
> >
> > Noel, if this works for you please push !
> >
> I think I am fine with all the changes with the exception of the code
> that removes the max_data passed to fill quota_buffer. Providing
> max_data is necessary for the correct operation for fill_quota_buffer.
> 
> To explain more the client requests (SMB1 & SMB2) both have a member
> ReturnSingleEntry/ReturnSingle in the associated quota user query
> messages. Both SMB1/SMB2 try to fill the response buffer with the
> available quota entries if the value of ReturnSingleEntry/ReturnSingle
> is zero. The client will keep sending the quota user query messages
> until there are no more quota entries returned by the server (indicated
> by STATUS_NO_MORE_ENTRIES (smb2) or an empty response (smb1)) The server
> maintains state between calls and will continue the listing of quota
> entries from the point where it exceeded buffersize.
> 
> With your current version where I hardcoded the a smaller
> max_data_count  (note you'd get the same effect if you increased the
> number of users that are returned when enumerating quotas)
> 
> -       if (blob.length > max_data_count) {
> +       if (blob.length > 80) {
>                return NT_STATUS_BUFFER_TOO_SMALL;
>         }
> 
> I can get smbcquotas (and a windows client) to not display any quota
> info at all :-(
> 
> Now I have to admit that I also screwed up when implementing you
> suggestion re.
> 
>     uint32_t dsize = sizeof(info.next_entry_offset)
>             + sizeof(info.sid_length)
>             + sizeof(info.change_time)
>             + sizeof(info.quota_used)
>             + sizeof(info.quota_threshold)
>             + sizeof(info.quota_limit)
> 
> and associated checks and I lost the check I had previously that really
> detected max_data was exceeded :-(
> 
> For purposes of demonstration I attach the following traces
> 
> [1] smbcquotas-with-samba-server-artificial-maxdata-limit.pcapng
> [2] win2012-client-with-samba-server-artificial-maxdata-limit.pcapng
> [3] win2012-client-with-samba-server-artificial-maxdata-limit-jra.pcapng
> [4] smbcquotas-with-samba-server-artificial-maxdata-limit-jra.pcapng
> 
> traces 1 & 2 are with my patches (version 2) with the fixup to detect
> max_data exceeded, traces 3 & 4 are with your changes. Both a windows
> client and smbcquotas are used.
> 
> While you are correct some of the code doesn't supply max_data and is
> passing 0, in these cases though it is just the client code doing this.
> Specifically this is when the client code is filling out set quota
> information (which uses the same fill_quota_buffer code) to push out a
> singe query info entry. I don't believe it makes sense to force an
> artificial buffer limit (but yes we can do it if necessary,  what size
> to use though :/) when the client is pushing the set_query info into the
> request, I don't think we test the size of other client messages. The
> buffer limit in the server code though makes sense given the way it
> needs to work (e.g. server tries to stuff buffer (with predefined size)
> with as many *complete* query info entries as it can)
> 
> I've modified your patchset (see attached) I've left the patches with
> the changes in line (see patches with title [SQUASHME]) and if you are
> happy I will squash those into the preceeding commits.

OK Noel, thanks a *LOT* for that explaination and the
wireshark traces that helped :-).

I finally "get" how the max_data is meant to be used (sorry
for the confusion).

I'm good with this patchset with the SQUASHME's squashed
into the previous commits.

Reviewed-by: Jeremy Allison <jra at samba.org>

Please merge and push to master !

We should probably think about back-porting to
4.9.x also.

Cheers,

	Jeremy.