| Message ID | 20180219025609.21659-1-lsahlber@redhat.com |
|---|---|
| Headers | show |
| Series | fix smb3-encryption breakage when CONFIG_DEBUG_SG=y | expand |
Hi Ronnie, Ronnie Sahlberg <lsahlber@redhat.com> writes: > Steve, All, > > I think smb3-encryption has been broken for quite a while (forver?) > when we configure the kernel with CONFIG_DEBUG_SG="y". > > The problem is that with these extra checks, we can no longer pass a pointer > to a buffer that is allocated off the stack safely to sg_set_buf() as this > function will BUG_ON. It seems to me this sg_set_buf() is quite restrictive. Is it an actual bug to pass stack buffers or does it become one only if this debug thing is set? IIUC it's the latter. > We have a lot of places where cifs.ko does indeed pass a pointer to a > stack object to this function :-( and this patch will fix all the instances > I could find so far. Switching that many stack buffers to heap doesn't sound like a good idea, especially in hot paths :( pros: * we can use this scatter-gather table debug thing (how?). cons: * makes the code more complex: memory management in C is error-prone as you all know (memory leaks, use-after-free, extra error code paths to add if allocation fails, etc) * slower: heap allocation is not free (altho we should probably measure instead of speculate) I'd like to know more about this debug feature because I'm not sure why there is this requirement. In any case thanks for finding and looking at this. Cheers,
----- Original Message ----- > From: "Aurélien Aptel" <aaptel@suse.com> > To: "Ronnie Sahlberg" <lsahlber@redhat.com>, "linux-cifs" <linux-cifs@vger.kernel.org> > Cc: "Steve French" <smfrench@gmail.com> > Sent: Monday, 19 February, 2018 10:39:22 PM > Subject: Re: [PATCH 0/0] fix smb3-encryption breakage when CONFIG_DEBUG_SG=y > > Hi Ronnie, > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > Steve, All, > > > > I think smb3-encryption has been broken for quite a while (forver?) > > when we configure the kernel with CONFIG_DEBUG_SG="y". > > > > The problem is that with these extra checks, we can no longer pass a > > pointer > > to a buffer that is allocated off the stack safely to sg_set_buf() as this > > function will BUG_ON. > > It seems to me this sg_set_buf() is quite restrictive. Is it an actual > bug to pass stack buffers or does it become one only if this debug > thing is set? IIUC it's the latter. It is the latter. > > > We have a lot of places where cifs.ko does indeed pass a pointer to a > > stack object to this function :-( and this patch will fix all the > > instances > > I could find so far. > > Switching that many stack buffers to heap doesn't sound like a good > idea, especially in hot paths :( t be > > pros: > * we can use this scatter-gather table debug thing (how?). > > cons: > * makes the code more complex: memory management in C is error-prone as you > all know (memory leaks, use-after-free, extra error code paths to add > if allocation fails, etc) > * slower: heap allocation is not free (altho we should probably measure > instead of speculate) > We need to do something since otherwise any time someone compiles their kernel with this option, smb3enc will no longer work, and the cifs module will die by BUG_ON :-( Moving everything to be kzallocated is error prone as you say and also a bit heavy handed. One alternative I see could be to just bypass the check completely and call sg_set_page() directly instead of sg_set_buf() but that does not feel right either. > I'd like to know more about this debug feature because I'm not sure why > there is this requirement. In any case thanks for finding and looking at > this. > > Cheers, > > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 19, 2018 at 9:39 PM, Aurélien Aptel <aaptel@suse.com> wrote: > Hi Ronnie, > > Ronnie Sahlberg <lsahlber@redhat.com> writes: >> Steve, All, >> >> I think smb3-encryption has been broken for quite a while (forver?) >> when we configure the kernel with CONFIG_DEBUG_SG="y". >> >> The problem is that with these extra checks, we can no longer pass a pointer >> to a buffer that is allocated off the stack safely to sg_set_buf() as this >> function will BUG_ON. > > It seems to me this sg_set_buf() is quite restrictive. Is it an actual > bug to pass stack buffers or does it become one only if this debug > thing is set? IIUC it's the latter. > >> We have a lot of places where cifs.ko does indeed pass a pointer to a >> stack object to this function :-( and this patch will fix all the instances >> I could find so far. > > Switching that many stack buffers to heap doesn't sound like a good > idea, especially in hot paths :( > > pros: > * we can use this scatter-gather table debug thing (how?). I don't think it is so much what we gain or why we would want to use CONFIG_DEBUG_SG but rather if someone else enables this, for whatever reason, it will make cifs.ko BUG_ON :-( > > cons: > * makes the code more complex: memory management in C is error-prone as you > all know (memory leaks, use-after-free, extra error code paths to add > if allocation fails, etc) > * slower: heap allocation is not free (altho we should probably measure > instead of speculate) > > I'd like to know more about this debug feature because I'm not sure why > there is this requirement. In any case thanks for finding and looking at > this. > > Cheers, > > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ronnie sahlberg <ronniesahlberg@gmail.com> writes: > I don't think it is so much what we gain or why we would want to use > CONFIG_DEBUG_SG > but rather if someone else enables this, for whatever reason, it will > make cifs.ko BUG_ON :-( Ok, but assuming the requirements of sg are too strict, maybe it's best to fix the sg code instead of working around it in cifs.ko? Cheers,