Jeff Layton <jlayton@xxxxxxxxxx> writes: > On Wed, 2021-06-23 at 19:34 -0500, Steve French wrote: >> updated patch attached with Aurelien's suggestion. >> >> On Wed, Jun 23, 2021 at 7:17 AM Paulo Alcantara <pc@xxxxxx> wrote: >> > >> > Agreed. >> > >> > On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@xxxxxxxx> wrote: >> > > Steve French <smfrench@xxxxxxxxx> writes: >> > > > We weren't checking if tcon is null before setting dfs path, >> > > > although we check for null tcon in an earlier assignment statement. >> > > >> > > If tcon is NULL there is no point in continuing in that function, we >> > > should have exited earlier. >> > > >> > > If tcon is NULL it means mount_get_conns() failed so presumably rc will >> > > be != 0 and we would goto error. >> > > >> > > I don't think this is needed. We could change the existing check after >> > > the loop to this you really want to be safe: >> > > >> > > if (rc || !tcon) >> > > goto error; >> > > >> > > >> > > Cheers, >> >> >> > > I know this patch is ancient and the mainline code has marched on, but > it seems really suspicious to me. Yes, it is. > With this, we have cifs_mount returning 0, even though the superblock > hasn't been properly initialized. Is that expected? Shouldn't it return > an error in that case? No, that isn't expected. And yes, if @tcon would ever be NULL at that point, we should be returning an error instead. Otherwise we'd end up dereferencing a NULL @tcon while trying to get an inode for the root dentry later. However, by quickly looking at the old code -- on top of 162004a2f7ef -- I don't see how we'd end up having a NULL @tcon with rc == 0 as mount_get_conns() would return -errno if it couldn't get a tcon. Please correct me if I'm missing something. Whether it is possibile or not, the NULL @tcon check is certainly missing a 'rc = -ENOENT' or some other error before bailing out as you've pointed out. > The mount handling has morphed considerably since this patch went in, so > I can't really tell whether this was later fixed or not. I don't think there was a follow-up patch for that.
