Hey Christian, On Mon, Jan 27, 2014 at 07:53:56AM +0000, cristian.sto...@freescale.com wrote: > > Why should that be necessary? > The patch is concerned with 'close' being called with garbage input. > I'm not sure about the call chain here: is 'close' never called when 'open' > fails? What is the assumption of the caller?
Suppose you write some simple program that writes to a file. You use open() to get the fd, which fails (for whatever reason). Would you then call close()? :) > > I just did a quick test, simply assigning > > NULL to pcr instead of the kzalloc call - despite filp->private_data > > being garbage at the beginning of cryptodev_open, cryptodev_release is > > not being called when it fails with -ENOMEM. > [] > This looks like the caller assumes that nothing ever happened if 'open' > failed. In that case, any cleanup should be done in 'open'. What else should she assume? I bet you wouldn't either (see above). Actually, it's quite simple: open() returns a file descriptor, or -1 if the call fails. In the later case, there simply is no descriptor to close, no matter what the caller assumes. > > Instead, I see a more prominent potential memleak here: If one of the > > todo list item allocations fails, nothing is being freed. There probably > > should be a bunch of gotos to the end of the function to undo stuff that > > has been done so far. What do you think? > [] > Based on your observations, all clean-up should be done in 'open' to avoid > the possibility of memleaks because 'close' is not called. > However, if 'close' is ever called with garbage, then it will fail - so we > should cover that case too. It's one thing less to think about. I think we can savely assume that 'close' (or 'cryptodev_release' to be precise) is always called for a file descriptor (struct file in our case) that was opened successfully. How would you distinguish between valid data and garbage anyway? Say, filp->private_data is 0xdeadbeef, is that garbage or the address of allocated data? But back to the topic: for cryptodev_open(), I suggest using the demonised 'goto' like so: | if (!init_a()) | goto fail_a; | if (!init_b()) | goto fail_b; | return 0; | fail_b: | deinit_a(); | fail_a: | return -1; Feel free to give it a go, I'll happily review your patch. Otherwise let me know and I'll make something up on my own. Greetings, Phil _______________________________________________ Cryptodev-linux-devel mailing list Cryptodev-linux-devel@gna.org https://mail.gna.org/listinfo/cryptodev-linux-devel
